-
Notifications
You must be signed in to change notification settings - Fork 4
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
deploy: first take to handle bump deploy
command 🎉
#10
Conversation
Hey! I started to review, but I think that we should extract all the refactorings into another, dedicated PR. This would make it easier to review, and would probably create a cleaner history in our PR. WDYT? |
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 haven't finished my review (and I have to leave from now), but I though you'd be happy to directly get my first bunch of comments. I haven't review the command code, neither the tests.
I'm super excited to see this going to production soon :D
This commit adds an optional token private attribute on the Base command of our CLI to be able to send an authorization header when interacting with the Bump API.
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 left one important comment, about how we handle the token
. Besides this, LGTM. So if we agree on it, this can be merged. If not, let me know your point of view so we can take a decision.
Great work, it's really clean 👍
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! Thanks for the last changes. Let's merge this one too!
Similarly to #8 this PR adds the new
bump deploy
command with lots of simplification compared to the current CLI (same flags removed as in #8).Help section
Example usage
Remaining todos
documentation
andhub
parameters. Needs https://github.com/bump-sh/bump/pull/537 to be merged.POST /api/v1/versions
answers with an empty response). Depends on API validations in https://github.com/bump-sh/bump/pull/546Closes #3