-
Notifications
You must be signed in to change notification settings - Fork 19
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
improvement: split manifests for dev and release #95
Conversation
Here is the summary of changes. You are about to add 16 region tags.
This comment is generated by snippet-bot.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
region tags LGTM! may want to wait for others to stamp it before merging :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not obvious at a glance the difference between k8s-manifests
and kubernetes-manifests
before looking at them and opening various resources. Additionally, it looks like you have 2 or 3 different manifests to update every time you make a change, instead of having a single source of truth. This can be error-prone.
In Bank of Anthos (and soon Online Boutique as well) we have only two directories:
dev-kubernetes-manifests
(this is the latest dev / source of truth)kubernetes-manifests
(this is read only and is basically just a snapshot ofdev-kuberentes-manifests
from the last time themake-release.sh
ran
That means that users uses kubernetes-manifests
, developers uses dev-kubernetes-manifests
(and modify only that), and once a new release is ready, when they run make-release.sh
it snapshots dev-kubernetes-manifests
into kubernetes-manifests
to reflect that new versioning
https://github.com/GoogleCloudPlatform/bank-of-anthos/blob/main/release/make-release.sh#L46-L55
Thoughts on doing something similar?
@bourgeoisor Thank you very much for reviewing this fast.
Like explained in the Description
Looking at BoA, I see that it is still having multiple yamls in both directories. In any-case, I think it's a matter of keeping the resources nearby and having a consistent way of updating the files via the automation we choose. Scan and update one yaml or scan and update multiple yamls in the same directory. Personally I feel having the same look and feel across different environment helps identify diffs for an outsider.
IMHO, this is not straight-forward and evident for someone new to the repository. For a noogler onboarding they wouldn't recognized it easily if we didn't explain it or they read the readme. Same goes for the external contributors. I understand that this was a structure decided upon when the repos were created but I think we can adopt a better model. As already explained above, the root will only have one directory
I think the point of discussion is the structure and what makes it straight-forward. The rest as to how people will consume it will definitely require README guidance in either case. Let me know if the above thought arguments I had when planning this structure makes sense. |
I missed the bit where the As for the way the |
Sorry about the confusion on the Yes, the release process should be automated and definitely documented for others to execute it. However, it is still in the works. So as I go adding these improvements I will also open PRs for the release procedure and supported documentation. I don't have it ready just yet! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Caveats for merge:
- I have not thoroughly tested these changes
- Documentation on these changes + automation of the release manifests generation incoming in a later PR
- This is not breaking anyone's workflow without letting them know
@bourgeoisor - I made one additional commit (a96ec35) which changed the newly added manifests to point to the CI project in Also for re: not tested, changes 1 and 2 is making sure that the new manifests are used to deploy the PR cluster. So if it passes then we should be good. I have tested it locally too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description:
Changes:
Related Issues: