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

fix: ensure appset git generator works with private repo #9179

Merged
merged 3 commits into from
Apr 28, 2022
Merged

fix: ensure appset git generator works with private repo #9179

merged 3 commits into from
Apr 28, 2022

Conversation

rishabh625
Copy link
Contributor

@rishabh625 rishabh625 commented Apr 22, 2022 •
edited

Signed-off-by: rishabh625 rishabhmishra625@gmail.com

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

Fixes:- #8967

@rishabh625
fix: ensure appset git generator works with private repo
13c836e 
Signed-off-by: rishabh625 <rishabhmishra625@gmail.com>
@codecov
Copy link

codecov bot commented Apr 22, 2022 •
edited

Codecov Report

Merging #9179 (ff73a31) into master (fdcaa55) will increase coverage by 0.30%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #9179      +/-   ##
==========================================
+ Coverage   45.49%   45.79%   +0.30%     
==========================================
  Files         219      220       +1     
  Lines       25870    25865       -5     
==========================================
+ Hits        11770    11846      +76     
+ Misses      12462    12373      -89     
- Partials     1638     1646       +8
Impacted Files Coverage Δ
reposerver/askpass/common.go 100.00% <100.00%> (ø)
applicationset/services/scm_provider/github.go 75.29% <0.00%> (-5.89%) ⬇️
applicationset/services/scm_provider/gitea.go 74.11% <0.00%> (-3.00%) ⬇️
util/cert/cert.go 82.40% <0.00%> (-2.35%) ⬇️
applicationset/services/scm_provider/utils.go 83.67% <0.00%> (-0.24%) ⬇️
...is/applicationset/v1alpha1/applicationset_types.go 47.22% <0.00%> (ø)
server/server.go 54.23% <0.00%> (+0.15%) ⬆️
util/gpg/gpg.go 68.20% <0.00%> (+0.20%) ⬆️
util/cmp/stream.go 52.14% <0.00%> (+1.09%) ⬆️
cmd/argocd/commands/app.go 14.99% <0.00%> (+2.62%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdcaa55...ff73a31. Read the comment docs.

@crenshaw-dev
Copy link
Member

The readonly root problem should be fixed by this: #9183

@rishabh625
Copy link
Contributor Author

The readonly root problem should be fixed by this: #9183

thanks

@rishabh625 rishabh625 mentioned this pull request Apr 22, 2022
Closed
@rishabh625
Copy link
Contributor Author

Fixes:- 8967

@rishabh625
Added env variable for sock file
0fe622b 
Signed-off-by: rishabh625 <rishabhmishra625@gmail.com>
@rishabh625 rishabh625 changed the title fix: ensure appset git generator works with private repo and reverting readonly roots fix: ensure appset git generator works with private repo Apr 25, 2022
@rishabh625 rishabh625 marked this pull request as ready for review April 25, 2022 16:56
@rishabh625 rishabh625 changed the title fix: ensure appset git generator works with private repo fix: ensure appset git generator works with private repo and fixes gitea issue in general Apr 25, 2022
@rishabh625
Copy link
Contributor Author

rishabh625 commented Apr 26, 2022 •
edited

@jannfis @ishitasequeira @chetan-rns @crenshaw-dev : can u please review?

I havemerged this PR branch too as checks were failing

chetan-rns
chetan-rns reviewed Apr 26, 2022
View reviewed changes
applicationset/services/scm_provider/utils.go Outdated
@@ -141,6 +141,9 @@ func getBranches(ctx context.Context, provider SCMProviderService, repos []*Repo
for _, repo := range repos {
reposFilled, err := provider.GetBranches(ctx, repo)
if err != nil {
if checkBranchDoNotExist(err.Error()) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to check this in the current PR? I saw another PR for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this changes please comment here

I merged branch just to get CI pass

@rishabh625 rishabh625 changed the title fix: ensure appset git generator works with private repo and fixes gitea issue in general fix: ensure appset git generator works with private repo Apr 27, 2022
@rishabh625
Copy link
Contributor Author

please review this

@rishabh625
Retrigger CI pipeline
ff73a31 
Signed-off-by: rishabh625 <rishabhmishra625@gmail.com>
chetan-rns
chetan-rns reviewed Apr 28, 2022
View reviewed changes
cmd/argocd-applicationset-controller/commands/applicationset_controller.go
@@ -131,6 +132,7 @@ func NewCommand() *cobra.Command {
startWebhookServer(webhookHandler, webhookAddr)
}
askPassServer := askpass.NewServer()
go func() { errors.CheckError(askPassServer.Run(askpass.SocketPath)) }()
Copy link
Member

Choose a reason for hiding this comment

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

The PR looks good to me. Since askPassServer.Run(path string) accepts path as an argument. Wouldn't it be easy to pass different paths for both repo server and applicationset? Currently, the path would be /tmp/reposerver-ask-pass.sock for the applicationset unless explicitly changed by an env variable. Just thinking out loud...

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation @rishabh625. It makes sense now. Without an env/flag, the path will be fixed when the argocd-git-ask-pass command will be invoked.

chetan-rns
chetan-rns approved these changes Apr 28, 2022
View reviewed changes
Copy link
Member

@chetan-rns chetan-rns left a comment

Choose a reason for hiding this comment

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

LGTM!

jannfis
jannfis approved these changes Apr 28, 2022
View reviewed changes
Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @rishabh625 !

@jannfis jannfis merged commit fc06da0 into argoproj:master Apr 28, 2022
@rishabh625 rishabh625 mentioned this pull request May 4, 2022
Closed
5 tasks
@crenshaw-dev crenshaw-dev added the cherry-pick/2.3 Candidate for cherry picking into the 2.3 release branch label Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jannfis jannfis jannfis approved these changes

@chetan-rns chetan-rns chetan-rns approved these changes

Assignees
No one assigned
Labels
cherry-pick/2.3 Candidate for cherry picking into the 2.3 release branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rishabh625 @crenshaw-dev @jannfis @chetan-rns