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

e2e: avoid mandatory command line flags #75593

Merged
merged 3 commits into from Jul 9, 2019

Conversation

@pohly
Copy link
Contributor

commented Mar 22, 2019

What type of PR is this?

/kind bug

What this PR does / why we need it:

Tests should never directly add to the global command line, because
some users of the tests might not want them there. For example,
options might only get set directly from a config file.

To achieve that, e2e/framework/config, e2e/framework/viperconfig, and
e2e/framework/test_context.go avoid using the global flag set and
instead expect to be told by the caller which flag set to use.

The exception is framework.HandleFlags, which as before directly
implements global command line handling.

Which issue(s) this PR fixes:

Fixes #75590

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

E2E tests no longer add command line flags directly to the command line, test suites that want that need to be updated if they don't use HandleFlags
loading a -viper-config=e2e.yaml with suffix (introduced in 1.13) works again and now has a regression test

/sig testing
/cc @deads2k

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

@deads2k is this critical for you? Should we try to get this into 1.14.0?

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

/retest

@timothysc

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

/assign @timothysc

@deads2k

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

@deads2k is this critical for you? Should we try to get this into 1.14.0?

It's important, but since it was in 1.13 too, I don't think a backport is warranted. I'm more interested in establishing a pattern that allows for clean import and composition.

@pohly pohly force-pushed the pohly:e2e-no-global-flags branch from ca18279 to 05da075 Apr 1, 2019

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

/retest

@timothysc

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

So in testing commons we ideally want to get to a versioned component config style config and leverage that work and deprecate what is currently in the test.

/hold
/cc @mtaufen @luxas

@timothysc

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

@pohly - what would you like todo here?

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

@pohly - what would you like todo here?

I don't know what the status is regarding the generic config handling in Kubernetes. If this isn't usable for E2E testing or there is no-one available to make the necessary changes (I myself don't have the time, sorry), then I would prefer to merge this PR because it addresses the valid issue that @deads2k pointed out. I just haven't rebased because the PR was put on hold and wouldn't get merged.

@timothysc

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

I'm ok with merging the PR for the time being.

@pohly pohly force-pushed the pohly:e2e-no-global-flags branch from 05da075 to 9baa6d9 Jul 4, 2019

@pohly pohly force-pushed the pohly:e2e-no-global-flags branch from 9baa6d9 to 6499cd0 Jul 4, 2019

@pohly pohly force-pushed the pohly:e2e-no-global-flags branch from 6499cd0 to 8a074cf Jul 4, 2019

e2e: fix full path support when reading viper config file
Something changed in Viper such that it now returns the
ConfigFileNotFound error when the config file is not found, for
example when it is specified including the .yaml or .json suffix.
When the code was originally was written, it returned "Unsupported
Config Type".

Found when adding a unit test for this code (separate commit because
it depends on the flag changes).

@pohly pohly force-pushed the pohly:e2e-no-global-flags branch from 8a074cf to c644067 Jul 4, 2019

pohly added some commits Mar 22, 2019

e2e: avoid mandatory command line flags
Tests should never directly add to the global command line, because
some users of the tests might not want them there. For example,
options might only get set directly from a config file.

To achieve that, e2e/framework/config, e2e/framework/viperconfig, and
e2e/framework/test_context.go avoid using the global flag set and
instead expect to be told by the caller which flag set to use. Tests
that called flag directly either get updated or obsolete flags get
removed.

The exception is framework.HandleFlags, which as before directly
implements global command line handling.

This is a breaking change for test suites which do not use that
function (and only those): they now need to ensure that they copy
individual flags from tests. Because the RegisterCommonFlags prototype
has changed, test suite authors will notice due to the resulting
compilation errors.

@pohly pohly force-pushed the pohly:e2e-no-global-flags branch from c644067 to 1822895 Jul 4, 2019

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

/retest

@pohly

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

@timothysc: I finished the rebasing.

The new viper config test revealed that the corresponding feature (loading a viper config file with explicit file suffix) had broken since introducing it in 1.13. I added the necessary fix in a separate commit, just in case that this is something we want to backport (probably not, as usage of the broken feature should be extremely small, if anyone does it at all).

@timothysc
Copy link
Member

left a comment

/lgtm
/approve
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm and removed do-not-merge/hold labels Jul 8, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pohly, timothysc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fejta-bot

This comment has been minimized.

Copy link

commented Jul 8, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

2 similar comments
@fejta-bot

This comment has been minimized.

Copy link

commented Jul 9, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

commented Jul 9, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit a61006b into kubernetes:master Jul 9, 2019

23 checks passed

cla/linuxfoundation pohly authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
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.