-
Notifications
You must be signed in to change notification settings - Fork 26
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
clarify pre-release instuctions #130
Conversation
Example of how we can start including it in the tracking issue: freedomofpress/securedrop#5689 |
e82523a
to
c07791c
Compare
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, not merging yet in case you want to address the above comments.
yes, i will address both comments, plus I add one more change to include notifying the localization manager after the tag is pushed so the localization team can start translations |
c35377e
to
228ba2d
Compare
This looks ready for another review. I also made an update about checking for Tor release candidates with major bug fixes to communicate them to the team. (Currently I'm eyeballing |
@@ -95,7 +93,7 @@ Pre-Release | |||
script, you will need to pass it the new version in the format | |||
``<major>.<minor>.<patch>~rcN``:: | |||
|
|||
securedrop/bin/dev-shell ../update_version.sh <major>.<minor>.<patch>~rcN | |||
bin/dev-shell ../update_version.sh <major>.<minor>.<patch>~rcN |
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.
Why remove the securedrop
here? AFAICT securedrop/bin/dev-shell
is still the correct invocation when you're in the repo root (given that, somewhat confusingly, we have a subdirectory also called securedrop
)
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.
if you're in the repo root then the ../update_version.sh
is incorrect, and as far as i can tell, you have to be in the securedrop
directory for this script to work. this is how it worked for me anyway.
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 believe the ..
is correct from the point of view of the dev-shell container:
~/Code/securedrop $ securedrop/bin/dev-shell ls ..
SECURITY.md codecov.yml mypy.ini
CODE_OF_CONDUCT.md SOURCE_OFFER devops securedrop
CONTRIBUTING.md Vagrantfile git securedrop-admin
LICENSE admin install_files setup.py
MANIFEST.in ansible.cfg journalist_gui tails_files
Makefile build junit update_version.sh
README.md changelog.md molecule
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.
Running securedrop/bin/dev-shell ../update_version.sh 1.8.0~rc1
in the repo root has the expected behavior for me.
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.
oh i see it does work the way it was, but confusing when you look at the command because it looks like you are specifying a path from the parent directory that will not exist, i.e. ../update_version.sh
will take you the parent directory of the root, which does not contain update_version.sh
. the reason it works is because that path is given to the container. that's the confusing part.
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.
That's my understanding as well. I don't have an opinion about the preferred invocation, but if we change the instructions we'll have to be explicit about changing into the securedrop
subdirectory first.
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.
to avoid confusion, i'll update this to include cd'ing into securedrop - i think that's the clearest the way this is currently written to work. any objections?
LGTM, one question about a changed command invocation. |
228ba2d
to
6d162f9
Compare
for everything else (it's fine to leave the changelog entries with empty bullets). For | ||
example, if the release is 1.3.0, then you'll run:: | ||
|
||
./update_version.sh 1.4.0~rc1 |
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.
As a side note, when dch
is not installed, the script helpfully prints:
$ ./update_version.sh 1.8.0~rc1
dch is missing
run with securedrop/bin/dev-shell ../update_version.sh <version>
So I think this is a fine simplification, and folks who need the container can still find out how to use it that way.
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.
Looks great, thanks a lot for these improvements :)
6d162f9
to
ea024b3
Compare
(One more rebase completed due to strict repo settings) |
Status
Ready for review
Description of Changes