Skip to content

asset/manifests: add Scheduler cluster config #1405

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.

Sign up for GitHub

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

Jump to bottom
Merged
openshift-merge-robot merged 1 commit into from
Mar 12, 2019
Merged

asset/manifests: add Scheduler cluster config #1405

openshift-merge-robot merged 1 commit into from
Mar 12, 2019

Conversation

@sjenning
Copy link
Contributor

@sjenning sjenning commented Mar 12, 2019

Render the cluster config CR for Scheduler

@derekwaynecarr @ravisantoshgudimetla @abhinavdahiya

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 12, 2019
@sjenning sjenning force-pushed the add-scheduler-cr-render branch from 42ccd02 to ec79e26 Compare March 12, 2019 17:24
staebler
staebler reviewed Mar 12, 2019
View reviewed changes
pkg/asset/manifests/scheduler.go Outdated
Copy link
Contributor

@staebler staebler Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no dependency on the install config. This can be removed.

staebler
staebler reviewed Mar 12, 2019
View reviewed changes
pkg/asset/manifests/scheduler.go Outdated
Copy link
Contributor

@staebler staebler Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change ing to something more appropriate for this asset.

Copy link
Contributor Author

@sjenning sjenning Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bet you can't tell where I copied and pasted from?!

@sjenning sjenning force-pushed the add-scheduler-cr-render branch from ec79e26 to 9f86bb2 Compare March 12, 2019 17:33
staebler
staebler reviewed Mar 12, 2019
View reviewed changes
Copy link
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better for this to be generated from a template file under data/data/manifests/bootkube by an asset in pkg/asset/templates/content/bootkube.

@abhinavdahiya
Copy link
Contributor

abhinavdahiya commented Mar 12, 2019

It would be better for this to be generated from a template file under data/data/manifests/bootkube by an asset in pkg/asset/templates/content/bootkube.

All the other config.openshift.io manifests are currently like this.
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 12, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Mar 12, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, sjenning

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

The pull request process is described here

Details 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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2019
@staebler
Copy link
Contributor

staebler commented Mar 12, 2019

It would be better for this to be generated from a template file under data/data/manifests/bootkube by an asset in pkg/asset/templates/content/bootkube.

All the other config.openshift.io manifests are currently like this.
/lgtm

@abhinavdahiya OK. Can you help me to understand the distinction between manifests that should be included in the templates and those that should not so that I can stop giving poor recommendations?

@openshift-merge-robot openshift-merge-robot merged commit 5e34eed into openshift:master Mar 12, 2019
@wking
Copy link
Member

wking commented Mar 26, 2019

Does this actually do anything?

$ curl -s https://storage.googleapis.com/origin-ci-test/logs/release-openshift-origin-installer-e2e-aws-4.0/6149/artifacts/e2e-aws/installer/.openshift_install.log | grep Scheduler
... no hits ...

I think you need to add it as a dependency over here or similar.

@sjenning
Copy link
Contributor Author

sjenning commented Mar 26, 2019

@wking gah, this was on my list to figure out. thanks for reminding me. it goes here https://github.com/openshift/installer/blob/master/pkg/asset/manifests/operators.go#L58-L61

@sjenning sjenning mentioned this pull request Mar 26, 2019
Closed
@sjenning
Copy link
Contributor Author

sjenning commented Mar 26, 2019

#1461

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ravisantoshgudimetla ravisantoshgudimetla Awaiting requested review from ravisantoshgudimetla

@hardys hardys Awaiting requested review from hardys

@steveej steveej Awaiting requested review from steveej

1 more reviewer

@staebler staebler staebler left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@abhinavdahiya abhinavdahiya

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@sjenning @abhinavdahiya @openshift-ci-robot @staebler @wking @openshift-merge-robot