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

fix: git clone on non-default branch fails (Fixes #5629) #5630

Merged
merged 4 commits into from
Apr 14, 2021

Conversation

kennytrytek
Copy link
Contributor

  • Fixes Issue 5629.

Signed-off-by: Kenny Trytek kenneth.g.trytek@gmail.com

Checklist:

@codecov
Copy link

codecov bot commented Apr 8, 2021

Codecov Report

Merging #5630 (86641a6) into master (46ec302) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 86641a6 differs from pull request most recent head f24f932. Consider uploading reports for the commit f24f932 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5630      +/-   ##
==========================================
- Coverage   47.17%   47.12%   -0.05%     
==========================================
  Files         240      240              
  Lines       15060    15062       +2     
==========================================
- Hits         7104     7098       -6     
- Misses       7053     7059       +6     
- Partials      903      905       +2     
Impacted Files Coverage Δ
workflow/artifacts/git/git.go 41.48% <100.00%> (+1.27%) ⬆️
cmd/argo/commands/get.go 55.66% <0.00%> (-1.95%) ⬇️
cmd/argoexec/commands/emissary.go 48.43% <0.00%> (-1.57%) ⬇️

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 46ec302...f24f932. Read the comment docs.

@kennytrytek
Copy link
Contributor Author

I don't think this change should cause catastrophic failure of the tests. Closing/reopening to try again...

@kennytrytek kennytrytek closed this Apr 8, 2021
@kennytrytek kennytrytek reopened this Apr 8, 2021
@kennytrytek kennytrytek changed the title fix: git clone on non-default branch fails fix: git clone on non-default branch fails (Fixes #5629) Apr 8, 2021
@kennytrytek kennytrytek marked this pull request as ready for review April 8, 2021 17:20
@alexec
Copy link
Contributor

alexec commented Apr 8, 2021

Can you please add a test?

Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

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

.

@kennytrytek
Copy link
Contributor Author

@alexec:
Can you please add a test?

Added test to verify the revision used is in the expected format. A full e2e test seemed a bit heavy for this change.

 - Fixes Issue 5629.

Signed-off-by: Kenny Trytek <kenneth.g.trytek@gmail.com>
 - Add test to verify git checkout receives expected arguments.

Signed-off-by: Kenny Trytek <kenneth.g.trytek@gmail.com>
@kennytrytek
Copy link
Contributor Author

Rebased onto latest master. No merge conflicts.

@alexec
Copy link
Contributor

alexec commented Apr 13, 2021

Please fix lint.

 - fix lint

Signed-off-by: Kenny Trytek <kenneth.g.trytek@gmail.com>
@kennytrytek
Copy link
Contributor Author

Sorry, didn't realize there was a lint target locally; I'll be sure to do that before pushing for future changes.

@alexec
Copy link
Contributor

alexec commented Apr 13, 2021

Test unrelated to changes failed:

--- FAIL: TestDAGTemplateParallelismLimit (0.11s)
    operator_test.go:1519: 
        	Error Trace:	operator_test.go:1519
        	Error:      	Not equal: 
        	            	expected: 2
        	            	actual  : 4
        	Test:       	TestDAGTemplateParallelismLimit

Re-running build.

@alexec alexec merged commit ec3b82d into argoproj:master Apr 14, 2021
@simster7 simster7 mentioned this pull request Apr 19, 2021
50 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants