Skip to content

Validate the ignition config before using it#1979

Closed
asalkeld wants to merge 2 commits intoopenshift:masterfrom
asalkeld:ignition-validation
Closed

Validate the ignition config before using it#1979
asalkeld wants to merge 2 commits intoopenshift:masterfrom
asalkeld:ignition-validation

Conversation

@asalkeld
Copy link
Contributor

@asalkeld asalkeld commented Jul 12, 2019

This should save time by not having to boot up the bootstrap node.

closes #1300

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 12, 2019
@abhinavdahiya
Copy link
Contributor

abhinavdahiya commented Jul 12, 2019

would proabably also need to validate on load...
https://github.com/openshift/installer/blob/62658de54672b1f1a4ee46ba1ddb643579f8e624/pkg/asset/ignition/bootstrap/bootstrap.go#L465

@asalkeld
Copy link
Contributor Author

asalkeld commented Jul 12, 2019

would proabably also need to validate on load...

https://github.com/openshift/installer/blob/62658de54672b1f1a4ee46ba1ddb643579f8e624/pkg/asset/ignition/bootstrap/bootstrap.go#L465

Cool, thanks. I'll do that.

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 12, 2019
@asalkeld
Copy link
Contributor Author

asalkeld commented Jul 12, 2019

fixing the vendor error..

@asalkeld
Copy link
Contributor Author

asalkeld commented Jul 12, 2019

/retest

@markmc
Copy link
Contributor

markmc commented Jul 12, 2019

Nice. Failed test happens after cluster bringup

/lgtm

/test e2e-aws-scaleup-rhel7

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

abhinavdahiya commented Jul 12, 2019

/test e2e-aws-scaleup-rhel7

is non-failing and we are currently fixing it, it can be ignored as it's not required for merge.

abhinavdahiya
abhinavdahiya reviewed Jul 12, 2019
View reviewed changes
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 14, 2019
@asalkeld
Copy link
Contributor Author

asalkeld commented Jul 15, 2019

/test e2e-aws

markmc
markmc reviewed Jul 15, 2019
View reviewed changes
pkg/asset/ignition/bootstrap/bootstrap_test.go Outdated
Copy link
Contributor

@markmc markmc Jul 15, 2019

Choose a reason for hiding this comment

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

Might have made sense to use the marshaled json here to avoid the marshaling and and to allow testing invalid json

Copy link
Contributor Author

@asalkeld asalkeld Jul 16, 2019

Choose a reason for hiding this comment

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

ok, I'll change that and test the Marshal error path too. Thanks for the review!

markmc
markmc reviewed Jul 15, 2019
View reviewed changes
Copy link
Contributor

@markmc markmc left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2019
abhinavdahiya
abhinavdahiya reviewed Jul 15, 2019
View reviewed changes
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 16, 2019
markmc
markmc reviewed Jul 16, 2019
View reviewed changes
Copy link
Contributor

@markmc markmc left a comment

Choose a reason for hiding this comment

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

/lgtm

pkg/asset/ignition/bootstrap/bootstrap_test.go Outdated
Copy link
Contributor

@markmc markmc Jul 16, 2019

Choose a reason for hiding this comment

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

a dup of the previous case?

pkg/asset/ignition/bootstrap/bootstrap_test.go Outdated
Copy link
Contributor

@markmc markmc Jul 16, 2019

Choose a reason for hiding this comment

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

at some point testing internal ignition error strings will get brittle and changes in the library will break the tests - the 3 error strings you have a probably fine though

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

asalkeld commented Jul 17, 2019

/test e2e-aws
/test e2e-aws-scaleup-rhel7

abhinavdahiya
abhinavdahiya reviewed Jul 17, 2019
View reviewed changes
pkg/asset/ignition/bootstrap/bootstrap_test.go Outdated
Copy link
Contributor

@abhinavdahiya abhinavdahiya Jul 17, 2019

Choose a reason for hiding this comment

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

installer/pkg/types/validation/installconfig_test.go

Lines 603 to 607 in 878dd9d

if tc.expectedError == "" {
assert.NoError(t, err)
} else {
assert.Regexp(t, tc.expectedError, err)
}

is more clear in the expectations that if error is required the error matches, and if it's not it should be NoError.

Copy link
Contributor Author

@asalkeld asalkeld Jul 17, 2019

Choose a reason for hiding this comment

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

ok, will do

@asalkeld
Validate the ignition config before using it
c03f79f 
This should save time by not having to boot up the bootstrap node.

closes #1300
@asalkeld
Add a test case for bootstrap ignition Load()
9d4ad80 
This is to make sure it correctly fails validation
@openshift-ci-robot openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 18, 2019
@markmc
Copy link
Contributor

markmc commented Jul 18, 2019

/lgtm

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jul 18, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: asalkeld, markmc
To complete the pull request process, please assign smarterclayton
You can assign the PR to them by writing /assign @smarterclayton in a comment when ready.

The full list of commands accepted by this bot can be found 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

@asalkeld
Copy link
Contributor Author

asalkeld commented Jul 19, 2019

/test e2e-aws-scaleup-rhel7

3 similar comments
@asalkeld
Copy link
Contributor Author

asalkeld commented Jul 22, 2019

/test e2e-aws-scaleup-rhel7

@asalkeld
Copy link
Contributor Author

asalkeld commented Aug 1, 2019

/test e2e-aws-scaleup-rhel7

@asalkeld
Copy link
Contributor Author

asalkeld commented Aug 19, 2019

/test e2e-aws-scaleup-rhel7

@asalkeld
Copy link
Contributor Author

asalkeld commented Aug 19, 2019

@abhinavdahiya can we get this merged please?

@abhinavdahiya
Copy link
Contributor

abhinavdahiya commented Aug 19, 2019

@abhinavdahiya can we get this merged please?

Oh, I'm sorry this dropped from my radar. Currently we are preparing for openshift 4.2 release and only bug fixes are being merged to the master.. as soon as we are ready to merge code to master for openshift 4.3 i will tag this. Thanks for your patience!

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 31, 2020

@asalkeld: PR needs rebase.

Details

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.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 31, 2020
@abhinavdahiya abhinavdahiya added version/4.6 Tracking changes that should end up in 4.6 release and removed version/4.5 labels May 8, 2020
@markmc markmc removed their assignment May 9, 2020
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Aug 4, 2020

@asalkeld: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-disruptive 9d4ad80 link /test e2e-aws-disruptive
ci/prow/yaml-lint 9d4ad80 link /test yaml-lint
ci/prow/tf-lint 9d4ad80 link /test tf-lint
ci/prow/shellcheck 9d4ad80 link /test shellcheck
ci/prow/e2e-aws-upgrade 9d4ad80 link /test e2e-aws-upgrade
ci/prow/govet 9d4ad80 link /test govet
ci/prow/gofmt 9d4ad80 link /test gofmt
ci/prow/verify-vendor 9d4ad80 link /test verify-vendor
ci/prow/images 9d4ad80 link /test images
ci/prow/golint 9d4ad80 link /test golint
ci/prow/unit 9d4ad80 link /test unit
ci/prow/verify-codegen 9d4ad80 link /test verify-codegen
ci/prow/e2e-aws-workers-rhel7 9d4ad80 link /test e2e-aws-workers-rhel7

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@abhinavdahiya
Copy link
Contributor

abhinavdahiya commented Oct 5, 2020

Closing this as it has stalled for very long. Please feel free to re-open in case you intent to keep pushing it.

/close

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 5, 2020

@abhinavdahiya: Closed this PR.

Details

In response to this:

Closing this as it has stalled for very long. Please feel free to re-open in case you intent to keep pushing it.

/close

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.

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

Reviewers

@jcpowermac jcpowermac Awaiting requested review from jcpowermac

@jhixson74 jhixson74 Awaiting requested review from jhixson74

2 more reviewers

@markmc markmc markmc left review comments

@abhinavdahiya abhinavdahiya abhinavdahiya left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. version/4.6 Tracking changes that should end up in 4.6 release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Ignition configs should be validated

4 participants

@asalkeld @abhinavdahiya @markmc @openshift-ci-robot