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.

Sign up for GitHub

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

Jump to bottom

Support for overriding base branch names #64

Merged
merged 6 commits into from
Feb 18, 2022
Merged

Support for overriding base branch names #64

merged 6 commits into from
Feb 18, 2022

Conversation

nirnanaaa
Copy link
Contributor

@nirnanaaa nirnanaaa commented Jan 3, 2022 •
edited
Loading

Support overriding base branch name (Follow up to #22)

Description

Since our git workflow uses multiple branches of the same repository in some places, I thought it would make sense to support an optional argument for overriding the base branch name globally (might be an addition to #22).

Documentation

  • README
  • --base-branch-name=<xxx>

TODOs

Please ensure all of these TODOs are completed before asking for a review.

  • Ensure the branch is named correctly with the issue number. e.g: feature/new-vpc-endpoints-955 or bug/missing-count-param-434.
  • Update the docs.
  • Keep the changes backward compatible where possible.
  • Run the pre-commit checks successfully.
  • Run the relevant tests successfully.

Related Issues

#22

@nirnanaaa nirnanaaa changed the title Feature/base branch name 22 Support for overriding base branch names Jan 3, 2022
@zackproser
Copy link
Contributor

Thanks so much for your contribution, @nirnanaaa! I'll take a look at this shortly.

zackproser
zackproser reviewed Jan 5, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
@nirnanaaa @zackproser
incorporate suggestions from review
6c88a8a 
Co-authored-by: Zack Proser <zackproser@gmail.com>
@zackproser
Copy link
Contributor

The implementation mostly looks good to me! I added some code necessary for allowing the parsing of the new --base-branch-name flag.

In local testing, I found one issue that I'll need to fix prior to us merging this in: namely the behavior where we key off an HTTP 422 status code from Github whenever a pull request fails to be opened.

Currently, we're using this return code to determine when someone has requested a draft pull request via the --draft flag on a repo that does not support drafts. However, Github's API also returns a 429 when the base branch target is invalid - meaning the user specifies a base branch that does not exist.

Before we can merge this, I'll need to add logic to determine which type of HTTP 429 error we received: a) the error relating to draft pull requests or b) the error relating to the base branch name provided by the user being invalid.

@zackproser
Differentiate between Github 422 errors
3df49f1 
- Github returns HTTP status 422 for several different error states
- Now that we're introducing --base-branch-name, we need to
differentiate between the error states
@zackproser
Copy link
Contributor

The implementation mostly looks good to me! I added some code necessary for allowing the parsing of the new --base-branch-name flag.

In local testing, I found one issue that I'll need to fix prior to us merging this in: namely the behavior where we key off an HTTP 422 status code from Github whenever a pull request fails to be opened.

Currently, we're using this return code to determine when someone has requested a draft pull request via the --draft flag on a repo that does not support drafts. However, Github's API also returns a 429 when the base branch target is invalid - meaning the user specifies a base branch that does not exist.

Before we can merge this, I'll need to add logic to determine which type of HTTP 429 error we received: a) the error relating to draft pull requests or b) the error relating to the base branch name provided by the user being invalid.

Alright - did this in 1adbd58. Kicking off tests now and then will look for someone who's not me to review!

pete0emerson
pete0emerson approved these changes Feb 18, 2022
View reviewed changes
Copy link

@pete0emerson pete0emerson left a comment

Choose a reason for hiding this comment

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

🚢

@zackproser
Copy link
Contributor

Thanks for the review! Going to merge this in now.

@zackproser zackproser merged commit 3dc1d1a into gruntwork-io:master Feb 18, 2022
@muhannad0 muhannad0 mentioned this pull request Dec 25, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zackproser zackproser zackproser left review comments

@pete0emerson pete0emerson pete0emerson approved these changes

@brikis98 brikis98 Awaiting requested review from brikis98 brikis98 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nirnanaaa @zackproser @pete0emerson