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

Use auth info (if present) when updating pipelines #62

Merged
merged 5 commits into from
Aug 1, 2018

Conversation

bidhan-a
Copy link
Contributor

I was getting a lot of these errors since my pipelines are in a git repo which requires authetication

screenshot

This PR should fix that.

Please let me know your thoughts.

Thanks!

@codecov-io
Copy link

codecov-io commented Jul 28, 2018

Codecov Report

Merging #62 into master will increase coverage by 0.97%.
The diff coverage is 59.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
+ Coverage   53.61%   54.58%   +0.97%     
==========================================
  Files          14       14              
  Lines        1218     1222       +4     
==========================================
+ Hits          653      667      +14     
+ Misses        496      486      -10     
  Partials       69       69
Impacted Files Coverage Δ
pipeline/ticker.go 0% <0%> (ø) ⬆️
pipeline/git.go 58.62% <66.66%> (+14.04%) ⬆️

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 3d8da40...4c9b516. Read the comment docs.

@bidhan-a bidhan-a changed the title Use auth info (if present) while updating pipelines Use auth info (if present) when updating pipelines Jul 28, 2018
@Skarlso
Copy link
Member

Skarlso commented Jul 28, 2018

Looks good! Can you add tests please? :) The policy is, that no code change or code goes in without tests. :)

@bidhan-a
Copy link
Contributor Author

bidhan-a commented Jul 28, 2018

Sure. How should we add tests for repos that require authentication? I couldn't find any test cases in the current codebase that deal with such repos. @Skarlso @michelvocks

@Skarlso
Copy link
Member

Skarlso commented Jul 28, 2018

@bidhan-a hmm, you'll have to take a look at the git repo framework. see how they do it in their test cases.

Ultimately you can do an integration test with actual credentials. Create a test user and test repo for it. But that would a last resort I think.

Otherwise, this might be a good chance to do #44 :D

@michelvocks
Copy link
Member

Looks good @bidhan-a 🤗
Would be awesome if you could provide some tests. 😄

@bidhan-a
Copy link
Contributor Author

bidhan-a commented Jul 29, 2018

@Skarlso I couldn't find any test case in go-git which handles repos that require authentication 😕

It seems like we'll have to look into other options.

@Skarlso
Copy link
Member

Skarlso commented Jul 29, 2018

@bidhan-a Crap. I'll take a look around later today. See if I can find something.

@Skarlso
Copy link
Member

Skarlso commented Jul 29, 2018

@bidhan-a Oh wait. I think we actually don't have to. Just test this function in the three ways:
https://github.com/gaia-pipeline/gaia/pull/62/files#diff-8b0b0b31212c3aeba36c22017d34614dR144

First, if you provide a username and password the authentication will be that. If not, than there is a public key auth and if neither then nil, nil is returned. We don't have to test github authentication process in this PR because frankly, we don't care. It's a third party component.

We can think about an integration test, but that's minuscule compared to three things we can test now. How does that sound?

@bidhan-a
Copy link
Contributor Author

@Skarlso Yep, that sounds good to me. Thanks! 😁

@@ -156,3 +156,60 @@ func TestUpdateAllPipelinesHundredPipelines(t *testing.T) {
t.Fatal("log output did not contain error message that the repo is up-to-date.: ", b.String())
}
}

func TestGetAuthInfo(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Would you be so kind as to extract these into three smaller tests? Or, if you want the setup, than use t.Run("bla", func() {}) in order to have three legs in one test while still using the setups.

@michelvocks
Copy link
Member

Good job, @bidhan-a 🤗 In my opinion this is ready to merge. @bidhan-a @Skarlso any objections?

@michelvocks michelvocks merged commit fa633e1 into gaia-pipeline:master Aug 1, 2018
@bidhan-a bidhan-a deleted the update-pipeline branch August 10, 2018 16:19
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.

4 participants