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

git: add PushConfig to specify push config and add support for refspecs #550

Merged
merged 3 commits into from May 15, 2023

Conversation

aryan9600
Copy link
Member

@aryan9600 aryan9600 commented May 10, 2023

Add PushConfig for configuring a push operation. Users can use PushConfig.Refspecs to specify the refspecs when using Push().

Furthermore, fix a bug related to Push() where all refs were pushed to origin, since we did not specify a refspec and the default refspec used by gogit is refs/heads/*:/refs/heads/*.

Add PushConfig.Force for force pushing and remove gogit.Client.forcePush in favor of the former.

Remove some stale test cases from TestSwitchBranch related to force push since we don't check if force pushing is enabled while switching branches anymore. Ref: #433

Part of: fluxcd/image-automation-controller#509

@hiddeco
Copy link
Member

hiddeco commented May 10, 2023

Think this approach will become difficult when we add support for push options, as there are now multiple methods which would require the additional option as an argument.

@aryan9600
Copy link
Member Author

Main reason why I added a new function is that I didn't want to break the API. But your point regarding push options is certainly valid. How about we add a new type PushConfig which would store refspecs, options (in the future), etc. and change Push(ctx) error to Push(ctx, pushConfig) error?

@hiddeco
Copy link
Member

hiddeco commented May 11, 2023

The second option would be to introduce an option pattern, which wouldn't break the existing API from a consumer perspective except for breaking the interface.

@stefanprodan
Copy link
Member

The git package is not v1 so I would not worry about breaking changes, let's chose the best UX even if it means changing the functions args.

@hiddeco hiddeco added enhancement New feature or request area/git Git and SSH related issues and pull requests labels May 11, 2023
@aryan9600
Copy link
Member Author

If we don't have any problems breaking API, I don't see the harm in just passing the config object directly as an argument. It also follows the pattern for Clone() where we pass a CloneOptions object directly. One thing to note is that I suggested PushConfig on purpose as calling it PushOptions could (and probably would) lead to confusion given the existence of Git push options.

@hiddeco
Copy link
Member

hiddeco commented May 11, 2023

If we go that route, I would maybe even go as far as renaming CloneOptions to CloneConfig to be annoying once instead of annoying forever in terms of naming differences. But otherwise, this sounds good to me.

@aryan9600
Copy link
Member Author

cool sounds good to me as well

@aryan9600 aryan9600 force-pushed the push-to-ref branch 3 times, most recently from 1799dac to 09a63be Compare May 11, 2023 11:39
git/gogit/client.go Outdated Show resolved Hide resolved
git/gogit/client.go Outdated Show resolved Hide resolved
git/gogit/client.go Outdated Show resolved Hide resolved
@aryan9600 aryan9600 changed the title git: add support for specifying refspecs for a push git: add PushConfig to specify push config and add support for refspecs May 12, 2023
@aryan9600 aryan9600 requested a review from pjbgf May 12, 2023 09:12
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @aryan9600 🙇

Add `PushConfig` for configuring a push operation. Users can use
`PushConfig.Refspecs` to specify the refspecs when using `Push()`.

Furthermore, fix a bug related to `Push()` where all refs were pushed to
origin, since we did not specify a refspec and the default refspec used
by gogit is `refs/heads/*:/refs/heads/*`.

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
Add `PushConfig.Force` for force pushing and remove
`gogit.Client.forcePush` in favor of the former.

Remove some stale test cases from `TestSwitchBranch` related to force
push since we don't check if force pushing is enabled while switching
branches anymore. Ref: #433

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
@aryan9600
Copy link
Member Author

talked to @pjbgf offline and he's okay with the removal of the obsolete tests

@aryan9600 aryan9600 merged commit bf62fd3 into main May 15, 2023
13 checks passed
@aryan9600 aryan9600 deleted the push-to-ref branch May 15, 2023 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/git Git and SSH related issues and pull requests enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants