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

integration-cli: don't build -test images if they already exist #38853

Merged
merged 3 commits into from Mar 27, 2019

Conversation

Projects
None yet
7 participants
@cyphar
Copy link
Contributor

commented Mar 12, 2019

There's no need to try to re-build the test images if they already
exist. This change makes basically no difference to the upstream
integration test-suite running, but for users who want to run the
integration-cli suite on a host machine (such as distributions doing
tests) this change allows images to be pre-loaded such that compilers
aren't needed on the test machine.

Signed-off-by: Aleksa Sarai asarai@suse.de

@cyphar cyphar force-pushed the cyphar:integration-cli-ensureImage branch from ea5d1e3 to ef4430c Mar 12, 2019

@cyphar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

The CI error is because interfacer is complaining about the interface type, but the developer behind interfacer states on the project README:

Deprecated: A tool that suggests interfaces is prone to bad suggestions, so its usefulness in real code is limited. This tool will remain available as a proof of concept, and for others to examine and learn from.

So I suggest we disable this linting tool, since it is known to produce bad suggestions (and there's no way to disable bad suggestions). Either that or I migrate all testingT usage to TestingT usage.

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

ping @vdemeester PTAL

@vdemeester

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

The CI error is because interfacer is complaining about the interface type, but the developer behind interfacer states on the project README:

Deprecated: A tool that suggests interfaces is prone to bad suggestions, so its usefulness in real code is limited. This tool will remain available as a proof of concept, and for others to examine and learn from.

So I suggest we disable this linting tool, since it is known to produce bad suggestions (and there's no way to disable bad suggestions). Either that or I migrate all testingT usage to TestingT usage.

oh… yeah either works for me to be honest… if the tool is deprecated it make sense to disable those suggestions

@vdemeester
Copy link
Member

left a comment

LGTM 🐯

@cpuguy83

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

+1 and/or we can use a type alias (assuming we want to continue to use it locally)
@cyphar Care to make the change?

@cyphar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

I will remove the usage of interfacer since that seems the best choice for long-term maintenance. The recommendation is also wrong, we can't use assert.TestingT -- because we have extra methods.

EDIT: I just checked and the extra methods are completely unused, so I will remove them and switch to TestingT.

@cyphar cyphar force-pushed the cyphar:integration-cli-ensureImage branch from ef4430c to f391e5d Mar 12, 2019

@cyphar cyphar requested a review from tianon as a code owner Mar 12, 2019

cyphar added some commits Mar 12, 2019

*: remove interfacer linter from CI
It has been declared deprecated by the author, and has a knack for
false-positives (as well as giving bad advice when it comes to APIs --
which is quite clear when looking at "nolint: interfacer" comments).

Signed-off-by: Aleksa Sarai <asarai@suse.de>
integration-cli: don't build -test images if they already exist
There's no need to try to re-build the test images if they already
exist. This change makes basically no difference to the upstream
integration test-suite running, but for users who want to run the
integration-cli suite on a host machine (such as distributions doing
tests) this change allows images to be pre-loaded such that compilers
aren't needed on the test machine.

However, this does remove the accidental re-compilation of nnp-test, as
well as handling errors far more cleanly (previously if an error
occurred during a test build, further tests won't attempt to rebuild
it).

Signed-off-by: Aleksa Sarai <asarai@suse.de>
internal: test/env: switch to assert.TestingT
Signed-off-by: Aleksa Sarai <asarai@suse.de>

@cyphar cyphar force-pushed the cyphar:integration-cli-ensureImage branch from f391e5d to ba0afa6 Mar 13, 2019

@codecov

This comment has been minimized.

Copy link

commented Mar 25, 2019

Codecov Report

Merging #38853 into master will decrease coverage by 0.27%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #38853      +/-   ##
==========================================
- Coverage   36.47%   36.19%   -0.28%     
==========================================
  Files         613      620       +7     
  Lines       45814    48298    +2484     
==========================================
+ Hits        16709    17482     +773     
- Misses      26823    28423    +1600     
- Partials     2282     2393     +111
@cyphar

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

/ping @cpuguy83

@cpuguy83
Copy link
Contributor

left a comment

LGTM

@cpuguy83

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

ping @vdemeester, can you give this a double-check with the new changes?

@vdemeester
Copy link
Member

left a comment

still LGTM 🐯

@vdemeester vdemeester merged commit da823cf into moby:master Mar 27, 2019

8 of 9 checks passed

codecov/patch 0% of diff hit (target 50%)
Details
codecov/project 36.19% (-0.28%) compared to f196671
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 44424 has succeeded
Details
janky Jenkins build Docker-PRs 53427 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 13635 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 24383 has succeeded
Details
windowsRS5-process Jenkins build Docker-PRs-WoW-RS5-Process 1725 has succeeded
Details
z Jenkins build Docker-PRs-s390x 13522 has succeeded
Details

@cyphar cyphar deleted the cyphar:integration-cli-ensureImage branch Mar 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.