Skip to content

Conversation

@umohnani8
Copy link
Member

make lint is complaining for cases where the error returned is checked
for err != nil, and then returned anyways.

Needed for containers/image#333

Signed-off-by: umohnani8 [email protected]

@mtrmac
Copy link
Contributor

mtrmac commented Sep 20, 2017

(LGTM.) Does this need vendoring containers/image#333, or are the two independent?

@umohnani8
Copy link
Member Author

containers/image#333 is failing due to skopeo failing, so I vendored it in here to make sure the tests pass once this error thing is fixed here.
Otherwise, it should be independent.

@mtrmac
Copy link
Contributor

mtrmac commented Sep 20, 2017

In that case it would be cleaner remove the vendoring, and commit this in skopeo separately; then the tests in containers/image#333 should pass.

@umohnani8 umohnani8 changed the title [DO NOT MERGE] fixing error checking due to update in make lint fixing error checking due to update in make lint Sep 20, 2017
@umohnani8
Copy link
Member Author

okay, fixed.

@umohnani8
Copy link
Member Author

actually nvm, it depends on containers/image#333 because there are some error checking fixes I made in containers/image.

@mtrmac
Copy link
Contributor

mtrmac commented Sep 20, 2017

AFAICS hack/validate-lint ignores everything in vendor/; is that broken?

@umohnani8
Copy link
Member Author

@mtrmac I am not sure. It failed when I took the vendor out and it was because of the containers/image error.

@mtrmac
Copy link
Contributor

mtrmac commented Sep 20, 2017

Seems to work fine for me in #423 .

make lint is complaining for cases where the error returned is checked
for err != nil, and then returned anyways.

Signed-off-by: umohnani8 <[email protected]>
@umohnani8
Copy link
Member Author

hmm, that is weird. I might have misread the error. I took the vendor out, lets see what happens.

@umohnani8
Copy link
Member Author

umohnani8 commented Sep 20, 2017

@mtrmac looks like the test is passing now, must have been a flake before?
Any reason why the second test is not running?

@mtrmac
Copy link
Contributor

mtrmac commented Sep 20, 2017

The second one is on macOS; Travis tends to have a backlog there lately, and today there’s some kind of outage ( https://www.traviscistatus.com ).

@umohnani8
Copy link
Member Author

Oh okay, looks like it started in #423, might start here soon.

@umohnani8
Copy link
Member Author

@mtrmac #423 is done. Do you want to merge that and I can close this PR?

@mtrmac
Copy link
Contributor

mtrmac commented Sep 20, 2017

You have done the original work, let’s merge yours to credit that.

@umohnani8
Copy link
Member Author

okay, thanks :)

@umohnani8
Copy link
Member Author

@mtrmac tests are done.

@TomSweeneyRedHat
Copy link
Member

LGTM and all green!

@rhatdan
Copy link
Member

rhatdan commented Sep 21, 2017

@runcom lets get this merged.

@mtrmac mtrmac merged commit 3c67427 into containers:master Sep 21, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants