Skip to content

Added yaml-lint shell script. Updated travis file to use yaml-lint sh… #101

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
Aug 3, 2018
Merged

Added yaml-lint shell script. Updated travis file to use yaml-lint sh… #101

openshift-merge-robot merged 1 commit into from
Aug 3, 2018

Conversation

@rabugopsl
Copy link
Contributor

@rabugopsl rabugopsl commented Aug 2, 2018

Added shell script to simplify invocation of yamllint task.

This task will be executed by Prow.
openshift/release/pull/1138

@coreosbot
Copy link

coreosbot commented Aug 2, 2018

Can one of the admins verify this patch?

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 2, 2018
wking
wking reviewed Aug 2, 2018
View reviewed changes
hack/yaml-lint.sh Outdated
Copy link
Member

@wking wking Aug 2, 2018

Choose a reason for hiding this comment

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

This is not an issue for Travis, but if we want to support devs running this locally, I think we want docker run -t --rm ... here.

Copy link
Contributor Author

@rabugopsl rabugopsl Aug 2, 2018

Choose a reason for hiding this comment

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

Done

wking
wking reviewed Aug 2, 2018
View reviewed changes
hack/yaml-lint.sh Outdated
Copy link
Member

@wking wking Aug 2, 2018

Choose a reason for hiding this comment

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

This would change the logic from what was in Travis, but we may want to lint modules as well. I'm not sure why that wasn't added when the Travis logic landed in coreos/tectonic-installer#3268.

Copy link
Contributor Author

@rabugopsl rabugopsl Aug 2, 2018

Choose a reason for hiding this comment

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

Added modules

Copy link
Member

@wking wking Aug 2, 2018

Choose a reason for hiding this comment

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

Added modules

Following up on this, @rabugopsl tried and turned up some errors. So we're punting on including modules for now, and we'll revisit after this PR lands.

wking
wking reviewed Aug 2, 2018
View reviewed changes
hack/yaml-lint.sh Outdated
Copy link
Member

@wking wking Aug 2, 2018

Choose a reason for hiding this comment

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

$ git show --check --oneline origin/pr/101
4136934 Added yaml-lint shell script. Updated travis file to use yaml-lint shell script
hack/yaml-lint.sh:2: trailing whitespace.
+if [ "$IS_CONTAINER" != "" ]; then

Copy link
Contributor Author

@rabugopsl rabugopsl Aug 2, 2018

Choose a reason for hiding this comment

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

removed trailing whitespace

@wking
Copy link
Member

wking commented Aug 2, 2018

/ok-to-test

@openshift-ci-robot openshift-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 2, 2018
@wking
Copy link
Member

wking commented Aug 2, 2018

The e2e-aws error was:

3 error(s) occurred:

* module.vpc.aws_route.to_nat_gw[3]: 1 error(s) occurred:

* aws_route.to_nat_gw.3: Error finding route after creating it: Unable to find matching route for Route Table (rtb-06425f78f948acba8) and destination CIDR block (0.0.0.0/0).
* module.vpc.aws_route.to_nat_gw[0]: 1 error(s) occurred:

* aws_route.to_nat_gw.0: Error finding route after creating it: Unable to find matching route for Route Table (rtb-06997bd3a24417c54) and destination CIDR block (0.0.0.0/0).
* module.vpc.aws_route.to_nat_gw[1]: 1 error(s) occurred:

* aws_route.to_nat_gw.1: Error finding route after creating it: Unable to find matching route for Route Table (rtb-00deab9b683160b7f) and destination CIDR block (0.0.0.0/0).

I'll leave it alone, and it will run a fresh test once you push updates.

@wking wking mentioned this pull request Aug 2, 2018
Closed
@wking
Copy link
Member

wking commented Aug 2, 2018 •
edited
Loading

The changes through 08f30ae4 look good to me. Can you squash down to a single commit? I don't think we need to preserve the whitespace removal, --rm addition, or aborted modules/ attempt for future readers :p. That's not a big deal though; I'm fine if approvers want to merge this PR without waiting for a squash.

@yifan-gu
Copy link
Contributor

yifan-gu commented Aug 3, 2018

+1, LGTM after squashing the commits. Thank you for the PR @rabugopsl !!

@petr-muller petr-muller mentioned this pull request Aug 3, 2018
Merged
@rabugopsl rabugopsl force-pushed the CORS-745_Move_yaml-lint_to_prow branch from 08f30ae to 9b80523 Compare August 3, 2018 12:23
@rabugopsl
Copy link
Contributor Author

rabugopsl commented Aug 3, 2018

Commits squashed

@wking
Copy link
Member

wking commented Aug 3, 2018

/lgtm

This needs to go in next with the release PR merged.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2018
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Aug 3, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rabugopsl, wking

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 Aug 3, 2018
@openshift-merge-robot openshift-merge-robot merged commit 54ae4d3 into openshift:master Aug 3, 2018
@wking wking mentioned this pull request Aug 3, 2018
Merged
@rabugopsl rabugopsl deleted the CORS-745_Move_yaml-lint_to_prow branch August 3, 2018 14:29
stbenjam pushed a commit to stbenjam/installer that referenced this pull request Feb 10, 2021
@openshift-merge-robot
Merge pull request openshift#101 from openshift-bot/art-consistency-o…
049dafb 
…penshift-4.8-ose-cluster-baremetal-operator

Updating ose-cluster-baremetal-operator builder & base images to be consistent with ART
clnperez added a commit to clnperez/installer that referenced this pull request Feb 1, 2022
@clnperez
match the machineset default image name (openshift#101)
4fea5f3 
match https://github.com/openshift-powervs/installer/blob/add-ipi/pkg/asset/machines/powervs/machinesets.go#L36

Signed-off-by: Christy Norman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wking wking wking left review comments

Assignees

@wking wking

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/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@rabugopsl @coreosbot @wking @yifan-gu @openshift-ci-robot @openshift-merge-robot