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

pkg/asset/installconfig/platform: Drop *PlatformType for types.{platform}.Name #659

Conversation

wking
Copy link
Member

@wking wking commented Nov 12, 2018

The old *PlatformType are from cccbb37 (#120), but since 476be07 (#573), we've had variables for the name strings in the more central pkg/types. With this pull request, we drop the more peripheral forms.

I've added a unit test to enforce sorting in PlatformNames, because the order is required by sort.SearchStrings in queryUserForPlatform.

This will have trivial conflicts with #657, but I'm fine rebasing after one of these lands.

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wking

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

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 12, 2018
@wking
Copy link
Member Author

wking commented Nov 12, 2018

e2e-aws:

error: watch closed before Until timeout
error openshift-ingress/ds/router-default did not come up
Waiting for daemon set "router-default" rollout to finish: 2 of 3 updated pods are available...
E1112 23:13:21.117169     713 streamwatcher.go:109] Unable to decode an event from the watch stream: net/http: request canceled (Client.Timeout exceeded while reading body)
error: watch closed before Until timeout
timeout waiting for openshift-ingress/ds/router-default to be available
2018/11/12 23:13:22 Container test in pod e2e-aws failed, exit code 1, reason Error

@abhinavdahiya
Copy link
Contributor

I've added a unit test to enforce sorting in PlatformNames, because the order is required by sort.SearchStrings in queryUserForPlatform.

I would expect the consumers to sort however they want, i don't think it should be part of the var Platforms api.

@wking
Copy link
Member Author

wking commented Nov 13, 2018

I've added a unit test to enforce sorting in PlatformNames, because the order is required by sort.SearchStrings in queryUserForPlatform.

I would expect the consumers to sort however they want, i don't think it should be part of the var Platforms api.

That's effectively "if you care about order, make a copy and sort it yourself", right? Why is that better than having an opinion here (e.g. alphabetical) and only require consumers to copy/sort when they need something else?

@abhinavdahiya
Copy link
Contributor

I've added a unit test to enforce sorting in PlatformNames, because the order is required by sort.SearchStrings in queryUserForPlatform.

I would expect the consumers to sort however they want, i don't think it should be part of the var Platforms api.

That's effectively "if you care about order, make a copy and sort it yourself", right? Why is that better than having an opinion here (e.g. alphabetical) and only require consumers to copy/sort when they need something else?

It's just odd that order is part of the Platforms api, and we are enforcing it with a test. But if you think that's appropriate, sure lets keep it.

@wking wking changed the title pkg/asset/installconfig/platform: Drop *PlatformType for types.PlatformName* pkg/asset/installconfig/platform: Drop *PlatformType for types.{platform}.Name Nov 13, 2018
@wking wking force-pushed the consolidate-package-name-variables branch from 9bf2fe6 to 6a04f63 Compare November 13, 2018 18:34
wking added a commit to wking/openshift-installer that referenced this pull request Nov 13, 2018
…orm}.Name

The old *PlatformType are from cccbb37 (Generate installation assets
via a dependency graph, 2018-08-10, openshift#120), but since 476be07
(pkg/asset: use vendored cluster-api instead of go templates,
2018-10-30, openshift#573), we've had variables for the name strings in the
more central pkg/types.  With this commit, we drop the more peripheral
forms.  I've also pushed the types.PlatformName{Platform} variables
down into types.{platform}.Name at Ahbinav's suggestion [1].

I've added a unit test to enforce sorting in PlatformNames, because
the order is required by sort.SearchStrings in queryUserForPlatform.

[1]: openshift#659 (comment)
@wking
Copy link
Member Author

wking commented Nov 13, 2018

Rebased around #657 and pushed the names down into the per-platform packages with 9bf2fe6 -> 1e129fe.

…orm}.Name

The old *PlatformType are from cccbb37 (Generate installation assets
via a dependency graph, 2018-08-10, openshift#120), but since 476be07
(pkg/asset: use vendored cluster-api instead of go templates,
2018-10-30, openshift#573), we've had variables for the name strings in the
more central pkg/types.  With this commit, we drop the more peripheral
forms.  I've also pushed the types.PlatformName{Platform} variables
down into types.{platform}.Name at Ahbinav's suggestion [1].

I've added a unit test to enforce sorting in PlatformNames, because
the order is required by sort.SearchStrings in queryUserForPlatform.

[1]: openshift#659 (comment)
@wking wking force-pushed the consolidate-package-name-variables branch from 6a04f63 to 1e129fe Compare November 13, 2018 18:36
@abhinavdahiya
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2018
@openshift-merge-robot openshift-merge-robot merged commit 8096592 into openshift:master Nov 13, 2018
@wking wking deleted the consolidate-package-name-variables branch November 13, 2018 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants