Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kustomize build CI + Kustomize docs + ServiceAccounts variation #1097

Merged
merged 36 commits into from
Sep 28, 2022

Conversation

mathieu-benoit
Copy link
Contributor

@mathieu-benoit mathieu-benoit commented Sep 26, 2022

  • Kustomize build CI for smoke tests as soon as files are updated in kustomize/ folder - Same approach as Add a small Terraform CI #1096 but for Kustomize
    • First, testing the default kustomization.yaml file in kustomize/
    • Second, testing each component individually under kustomize/components
    • Thirst, and last, testing a custom scenario with base and some components
  • Do not trigger current CI on each PR or merge in main if files are updated in kustomize/ folder to avoid charging the self-hosted runner as well as avoid building/deploying the containers, all of that not related to the Kustomize implementation in this repo as of now
  • Just one kubernetes-manifests.yaml in kustomize/base folder, like in release/
  • Update the release script/process to copy the release/kubernetes-manifests.yaml file in kustomize/base/kubernetes-manifests.yaml
  • Update the REDIS_ADDR value update for the Memorystore component (a Redis instance could have a port after the IP and it could also be a DNS instead of IP). Here, I'm proposing that any Kustomize component which needs to have value injection (sed) in their Kustomize component file is surrounded by {{...}}, in this case {{REDIS_ADDR}}. That's the only component for now having this, so that's kind of a token convention for future components which will need that in the future.
  • Add new ServiceAccounts variation via Kustomize
  • Lastly, update the docs for consistency between the Kustomize variations

@mathieu-benoit mathieu-benoit requested a review from a team as a code owner September 26, 2022 21:44
@github-actions
Copy link

🚲 PR staged at http://35.223.150.213

@mathieu-benoit mathieu-benoit marked this pull request as draft September 26, 2022 21:57
@github-actions
Copy link

🚲 PR staged at http://35.223.150.213

@github-actions
Copy link

🚲 PR staged at http://35.223.150.213

@github-actions
Copy link

🚲 PR staged at http://35.223.150.213

@github-actions
Copy link

🚲 PR staged at http://35.223.150.213

@mathieu-benoit mathieu-benoit marked this pull request as ready for review September 27, 2022 00:10
@github-actions
Copy link

🚲 PR staged at http://35.223.150.213

@github-actions
Copy link

🚲 PR staged at http://35.223.150.213

@github-actions
Copy link

🚲 PR staged at http://35.223.150.213

- name: kustomize build memorystore
run: |
cd kustomize/
kustomize edit add component components/memorystore
Copy link
Collaborator

@NimJay NimJay Sep 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question:
Each step is adding a Component and then running Kustomize-building.
My question: Is each step building on top of the previous step?
For instance, does the kustomize build google-cloud-operations step include the cymbal-branding Component?
Reason I ask: I would rather not build steps on top of the previous step. I think we should test each Component on its own. I think promising users that all the Components ALL components will work together is a big commitment. I think the CUJ we should 100% support and prioritize is "each Component can be deployed on its own".

Copy link
Contributor Author

@mathieu-benoit mathieu-benoit Sep 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ah, you just read my mind and my screen actually! Aligned with you, 100% agreed on that.

I will work on the draft mode to include this:

  • Do not repeat the steps, but have a loop in bash browsing the subfolders in components - with that, as soon as a new component is added, no need to update the CI definition
  • Each test will be done alone, just 1 component at a time
  • The last test will combined all the components, to have kind of a "test them all"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Let me know what you think! ;)

@mathieu-benoit mathieu-benoit marked this pull request as draft September 27, 2022 14:33
@github-actions
Copy link

🚲 PR staged at http://35.223.150.213

@github-actions
Copy link

🚲 PR staged at http://35.223.150.213

@github-actions
Copy link

🚲 PR staged at http://35.223.150.213

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
@github-actions
Copy link

🚲 PR staged at http://35.223.150.213

@github-actions
Copy link

🚲 PR staged at http://35.223.150.213

@github-actions
Copy link

🚲 PR staged at http://35.223.150.213

@github-actions
Copy link

🚲 PR staged at http://35.223.150.213

@github-actions
Copy link

🚲 PR staged at http://35.223.150.213

1 similar comment
@github-actions
Copy link

🚲 PR staged at http://35.223.150.213

@github-actions
Copy link

🚲 PR staged at http://35.223.150.213

Copy link
Collaborator

@NimJay NimJay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much, @mathieu-benoit, for these improvements.

I also highly appreciate that you configured the new Kustomize-related CI to only run if changes are made to /kustomize or .github/workflows/kustomize-build-ci.yaml.

I had a lot of nitpicks from my end — so I've gone ahead and committed them myself to avoid back-and-forth and to avoid wasting your time. 😄

Of course, please create smaller, more focused pull-requests in the future. 🙏
E.g., this pull-request could've been 3 (as suggested by the title).
It would really help me as a reviewer. 😊

I approve these changes!
Feel free to merge and/or undo any change(s) I made.

Once again, thank you!!! 🚀

@mathieu-benoit mathieu-benoit merged commit 06c4af7 into main Sep 28, 2022
@mathieu-benoit mathieu-benoit deleted the mathieu-benoit/kustomize-build-ci branch September 28, 2022 15:24
arbrown pushed a commit that referenced this pull request Sep 28, 2022
)

* Create kustomize-build-ci.yml

* Update ci-pr.yaml

* add license header

* kustomize build cymbal-branding

* Update kustomize-build-ci.yml

* Update kustomize-build-ci.yml

* Just one file for the Kustomize base (equivalent of release/)

* update the release script/process to copy the file from release/ to kustomize/base

* kubectl kustomize

* components/network-policy

* do not trigger ci-main for kustomize/ updates

* rename ci file for consistency

* test each component at first then eventually test all-in-one

* Update Kustomize for Memorystore to take into account more generic REDIS_ADDR

* kustomize build custom cuj instead of a generic all-in-one

* fix ci

* rename network-policy by network-policies under kustomize/

* Review Kustomize main doc

* update Kustomize main doc

* fix broken links

* Update Kustomize docs with consistency

* Formatting the kustomization.yaml files like kustomize create/edit do

* Add ServiceAccounts via Kustomize

* fix file path

* Update README.md

* Update kustomize/README.md

* Update headings in /kustomize/README.md

* Updae kustomize/ docs

* End ServiceAccounts with newline for cleaner cat

* Avoid use of Google-centric term, CUJ

* Update terraform/README.md

Co-authored-by: Nim Jayawardena <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants