-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,7 @@ concurrency: | |
on: | ||
push: | ||
branches: | ||
- 14/edge | ||
- "*/edge" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for so I think on 14/edge, this should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With a wildcard we don't have to maintain a permadiff between There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
I think this would be caught in PR reviews I think it's more likely that |
||
paths: | ||
- snap/** | ||
workflow_dispatch: | ||
|
@@ -25,7 +25,7 @@ jobs: | |
- build | ||
uses: canonical/data-platform-workflows/.github/workflows/[email protected] | ||
with: | ||
channel: 14/edge | ||
channel: ${{ github.ref_name }} | ||
artifact-prefix: ${{ needs.build.outputs.artifact-prefix }} | ||
secrets: | ||
snap-store-token: ${{ secrets.SNAP_STORE_TOKEN }} | ||
|
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
https://docs.renovatebot.com/configuration-options/
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