-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Revert "Revert "cluster-launch-installer-e2e: remove etcd nodes"" #1290
Revert "Revert "cluster-launch-installer-e2e: remove etcd nodes"" #1290
Conversation
This reverts commit a1b5732. Now that openshift/installer@0e65a68c (*: remove etcd nodes, 2018-08-16, openshift/installer#168) has landed, we can safely remove this.
Please add an appropriate OWNERS so you don't manually merge items to this repo. |
My plan for this is:
I'm reading this as "so you can approve your own release changes without bothering the rest of us non-installer folks" ;). If you mean something else, can you clarify? |
Right, that approach is not one that can happen for this PR so in the meanwhile can we please just add a list of
When @eparis or someone else manually merges a pull request with the button there is a chance that the PR was not tested against the latest |
The OWNERS file is copied from openshift/installer@d22a9870 (OWNERS: Promote abhinavdahiya and wking to approvers, 2018-08-02, openshift/installer#103), which is still current as of openshift/installer@245fc0f6 (Merge pull request openshift/installer#168 from crawford/etcd, 2018-08-27). Copy/pasting into this repo is not very DRY, but this the temporary approach Steve is requesting [1] while we work out something more maintainable. [1]: openshift#1290 (comment)
Pushed with 49f60b7.
Ah, I hadn't realized that happened, although in hindsight it seems obvious :p. And yeah, +1 to not needing that sort of thing. |
/lgtm whoever reviews can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: stevekuznetsov, wking, yifan-gu 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 |
/hold cancel |
@wking: Updated the
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Some slurped repositories use OWNERS_ALIASES [1], so we need to pull these in to resolve entries in their OWNERS. OWNERS_ALIASES can only exist in the repo root [2]. That means we may need to namespace the upstream aliases to avoid collisions when assembing the composite OWNERS_ALIASES here. Defining aliases in upstream repos will be useful for situations where a particular repo team (e.g. the installer team) should have control over other directories within this repositories. For example, $ cat cluster/test-deploy/aws/OWNERS approvers: - installer-approvers reviewers: - installer-reviewers is simpler and more maintainable with an auto-maintained OWNERS_ALIASES than the copy/paste we used in 49f60b7 (cluster/test-deploy/aws/OWNERS: Delegate to openshift/installer, 2018-08-27, openshift#1290). With script control over OWNERS_ALIASES, *this* repo can't define its own aliases. But that doesn't seem like a large limitation, because this repo is unlikely to need aliases that are not defined in *any* of the upstream repositories. And if we need to grow a location for local alias configuration, we can always add that in the future. I'm using 'git clone...' over SSH to access the files, because that provides access to private repositories. Using: https://raw.githubusercontent.com/{organization}/{repository}/HEAD/{path} only works if you have an auth token or the repository is public. Using clone for two files is a bit heavy. Ideally we could pull just the two files we're interested. Git's archive command supports that use-case, but GitHub doesn't seem to have enabled the server side of that transaction: $ git archive --format=tar --remote ssh://git@github.com/openshift/installer.git HEAD OWNERS OWNERS_ALIASES Invalid command: 'git-upload-archive '/openshift/installer.git'' You appear to be using ssh to clone a git:// URL. Make sure your core.gitProxy config option and the GIT_PROXY_COMMAND environment variable are NOT set. fatal: The remote end hung up unexpectedly $ git --version git version 1.8.3.1 Until they enable that, I'm fine consuming more of their bandwidth than is strictly necessary ;). Using reflect in assertEqual gives us a nice, compact implementation. But the output can be a bit hard to read. With an introduced error: $ go test . ... --- FAIL: TestOrgRepos (0.00s) main_test.go:15: unexpected result: [0xc42006e1e0 0xc42006e240] != [0xc42006e2a0] --- FAIL: TestGetOwners (2.30s) main_test.go:15: unexpected result: &{Directories:[] Organization:openshift Repository:installer Owners:0xc42006e420 Aliases:<nil> Commit:aaa47f6a54f0fa0449d36de01ccb9f56729b608a} != &{Directories:[] Organiza ... I'd mocked up an assertEqual based on comparing json.MarshalIndent strings which produced: --- FAIL: TestOrgRepos (0.00s) main_test.go:30: unexpected result: [ { "directories": [ "/tmp/populate-owners-389376240/a/b" ], "organization": "a", "repository": "b" }, { "directories": [ "/tmp/populate-owners-389376240/c/d" ], "organization": "c", "repository": "d" } ] != [ { "directories": [ "/tmp/populate-owners-389376240/a/b" ], "organization": "a", "repository": "b" } ] --- FAIL: TestGetOwners (2.38s) main_test.go:30: unexpected result: { "organization": "openshift", "repository": "installer", "owners": { "approvers": [ "aaronlevy", "abhinavdahiya", "crawford", "smarterclayton", "wking", "yifan-gu" ], "reviewers": [ "vikramsk" ] }, "commit": "aaa47f6a54f0fa0449d36de01ccb9f56729b608a" } != { "organization": "openshift", "repository": "installer", "owners": { "approvers": [ "aaronlevy", "abhinavdahiya", "crawford", "smarterclayton", "wking", "yifan-gu" ], "reviewers": [ "vikramsk" ] }, "commit": "2587b3ed493c18747f2b37e1ab6daebdd277631a" } and of course, there are a number of third-party libraries that do an even better job (e.g. [3]). But I don't think this functionality is worth an external dependency, and Steve doesn't think it's worth maintaining much local code for comparison [4], so we're going with the more compact reflect approach despite the less-useful output. And the tests should all be passing anyway, right? ;) The GIT_*_DATE tick business is based on [5]. I haven't bothered running a new commit after each test.setup change, because we don't need a different commit hash. In production, extractOwners will be run on a fresh clone, so we will always have a different commit hash if the content changes. The stub populate-owners.sh wraps the 'go run' at Steve's request [6]. We no longer need anything Bash-specific, so I'm using a POSIX shebang in the wrapper. The 'exec' allows us to replace the shell command with Go without executing a new process [7] (although Go itself will proceed to launch a few processes). The O_TRUNC is an attempt to avoid occasional corruption like: diff --git a/ci-operator/jobs/openshift/azure-misc/OWNERS b/ci-operator/jobs/openshift/azure-misc/OWNERS index 132c684..bf0dec5 100644 --- a/ci-operator/jobs/openshift/azure-misc/OWNERS +++ b/ci-operator/jobs/openshift/azure-misc/OWNERS @@ -3,10 +3,11 @@ # See the OWNERS docs: https://git.k8s.io/community/contributors/guide/owners.md approvers: +- pweil- - jim-minter - kargakis - kwoodson -- mjudeikis +is - pweil- reviewers: - jim-minter although the exact source of that corruption is not clear to me. [1]: https://github.com/openshift/cluster-api-provider-aws/blob/44e974de04f496f9552ecd37b73cad01b6d69f4d/OWNERS_ALIASES [2]: https://github.com/kubernetes/community/blame/0741bdfbb56dcd4829754560f79a2f3da32cb34f/contributors/guide/owners.md#L54 [3]: https://godoc.org/github.com/stretchr/testify/assert#Equal [4]: openshift#1285 (comment) [5]: https://github.com/git/git/blob/v2.18.0/t/test-lib-functions.sh#L128-L138 [6]: openshift#1285 (comment) [7]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#exec
The previous content was copy/pasted into this repo with 49f60b7 (cluster/test-deploy/aws/OWNERS: Delegate to openshift/installer, 2018-08-27, openshift#1290). But with upstream OWNERS_ALIASES being slurped since e1f993f (populate-owners: Also slurp OWNERS_ALIASES, 2018-08-25, openshift#1285) and the installer repo defining aliases since openshift/installer@62f87acb (OWNERS_ALIASES: Define aliases for reviewers and approvers, 2018-08-24, openshift/installer#184), we can DRY this up by using the upstream aliases.
This reverts commit a1b5732. Previous related PRs: #1284, #1274.
Now that openshift/installer@0e65a68c (*: remove etcd nodes, 2018-08-16, openshift/installer#168) has landed, we can safely remove this.
CC @yifan-gu, @eparis.