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

Removes branch-based conditional execution of CI rebase script #5424

Merged
merged 3 commits into from Jul 31, 2020

Conversation

zenmonkeykstop
Copy link
Contributor

@zenmonkeykstop zenmonkeykstop commented Jul 29, 2020

Status

Ready for review

Description of Changes

Updates references to defunct master branch in rebase-ci.sh.

Testing

  • CI passes

@zenmonkeykstop zenmonkeykstop changed the title Updates stray references to master branch in CI and test scripts Updates stray references to master branch in CI rebase script Jul 30, 2020
@zenmonkeykstop zenmonkeykstop changed the title Updates stray references to master branch in CI rebase script Removes stray references to master branch in CI rebase script Jul 30, 2020
@zenmonkeykstop zenmonkeykstop changed the title Removes stray references to master branch in CI rebase script Removes branch-based conditional execution of CI rebase script Jul 30, 2020
Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

LGTM. I see you rolled back your change to the i18n tool logic, which is now targeting upstream master, rather than main. @emkll is there any reason to hold off on a change like 10fe54b#diff-5c5e6b7fb4a6fb6bf1ea79d1c651e0fcL269-R269 ? If so, let's update the i18n repo and press on.

@zenmonkeykstop depending on how the i18n repo changes shake out, might be prudent to squash here. No concerns with the core logic changes you're making, they're quite clear.

@conorsch
Copy link
Contributor

is there any reason to hold off on a change like 10fe54b#diff-5c5e6b7fb4a6fb6bf1ea79d1c651e0fcL269-R269 ? If so, let's update the i18n repo and press on.

Just had a mindmeld with @ketudb, and they concluded that the "main" branch needs to be pushed to the "securedrop-i18n" fork, then the change that @zenmonkeykstop reverted will work agan. We don't need to slip that into this PR, so I'm not blocking merge, but would appreciate further input before we declare the branch migration process over and done with.

@zenmonkeykstop
Copy link
Contributor Author

There isn't actually a main branch on this repo, default is develop and master has effectively been defunct for a while (until the recent docs kerfuffle). Hence the revert.

@conorsch
Copy link
Contributor

@zenmonkeykstop Thanks for clarifying. Seems to me that the i18n script should be branching from develop, then, not from master (or main). Tangential for this PR, so fine to follow up later!

@conorsch conorsch merged commit f1b495e into develop Jul 31, 2020
SecureDrop Team Board automation moved this from Ready for Review to Done Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants