-
Notifications
You must be signed in to change notification settings - Fork 1
[MISC] Update PG version and dual branch configurations #5
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
Conversation
- 14/edge | ||
- "*/edge" |
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.
for on: push
, I believe GitHub uses the release.yaml from the latest commit on the branch
so I think on 14/edge, this should be 14/edge
and on 16/edge this should be 16/edge
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.
With a wildcard we don't have to maintain a permadiff between 14/edge
and 16/edge
.
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.
I think it could confuse developers into thinking the workflow file only needs to be edited once or thinking that the file applies to branches it doesn't
Also, given that we're diverging the branches, I expect we might diverge the workflow file at some point (e.g. when 14/edge is security only updates, it'll probably stay on an old version of dpw and any migrations for dpw breaking changes in the default branch wouldn't be synced to 14/edge)
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.
On the other hand, mixing up the permadiff when merging will break publishing. I don't think that the branches are expected to diverge CI wise for the foreseeable future, so I rather keep them as consistent as possible until we have a reason to do a hard split.
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.
mixing up the permadiff when merging
I think this would be caught in PR reviews
I think it's more likely that "*/edge"
will confuse someone into misunderstanding how the workflows work than the publishing accidentally getting broken
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.
imo we should only have the renovate config on the default branch, since renovate only reads it from the default branch
should the default branch be changed to 16/edge?
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.
Renovate can track multiple base branches eg dragomirp#3 and dragomirp#2
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.
yes, but it only reads config from default branch
Renovate always uses the config from the repository's default branch, even if that configuration specifies multiple
baseBranches
. Renovate does not read/override the config from within each base branch if present.
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.
I'd rather keep the two branches consistent, even if the second renovate config is ignored.
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.
I think this is confusing & will lead people to misunderstand the renovate config/apply changes to the renovate config in the wrong place
Also, it makes modifying the renovate config require 2 prs instead of 1
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.
imo, when our tooling around managing multiple branches for charms matures, I think we should revisit this (I think we will need divergence on the branches anyways and this approach will cause confusion, especially for new joiners)
but, after discussing with Alex, approving to unblock for now
Tested on changes on https://github.com/dragomirp/postgresql-snap