Skip to content

add version check to ci #211

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

Merged
merged 1 commit into from
Mar 13, 2019

Conversation

drubin
Copy link
Contributor

@drubin drubin commented Feb 10, 2019

Adds a version check to CI this is so we always ensure the package-lock matches the package version before merging. (its happened a few times)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 10, 2019
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 10, 2019
@drubin drubin changed the title add version check to ci WIP: add version check to ci Feb 10, 2019
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 10, 2019
@drubin drubin force-pushed the version-check-ci branch 2 times, most recently from 2d04f3b to 95c18f6 Compare February 10, 2019 12:20
@brendandburns
Copy link
Contributor

@drubin it looks like this didn't work? Because there's version skew but Travis passed...

@itowlson
Copy link
Contributor

It's detecting the version skew and emitting the log message, but not failing the build.

Should we be using process.exit(1)? Though the docs seem to imply that assigning process.exitCode should work.

@itowlson
Copy link
Contributor

It... it couldn't be some sort of horrible operator precedence issue could it? Like when it sees process.exitCode = 1 && console.log(...) it parses it as process.exitCode = (1 && console.log(...)) and because console.log(...) returns undefined that sets process.exitCode to 0 or undefined or something?

You wouldn't do that to us, would you JavaScript? Would you?

@itowlson
Copy link
Contributor

NARRATOR VOICE: It would

(process.exitCode = 1) && console.log(...) appears to work.

@itowlson
Copy link
Contributor

A colleague pointed out that -e does support statements not just expressions so if (...) { process.exitCode = 1; console.log(...); } might be easier for maintainers to read, though it does get a bit verbose.

@drubin
Copy link
Contributor Author

drubin commented Feb 12, 2019

Sorry, I should have included a better comment, It was something I quickly tried over the weekend. (the failure code worked locally) but for some reason on Travis, it didn't work.

But I will update it a bit later and see. (I will also force a failure so we can check it works)

Sorry for any spam.

@drubin
Copy link
Contributor Author

drubin commented Feb 12, 2019

A colleague pointed out that -e does support statements not just expressions so if (...) { process.exitCode = 1; console.log(...); } might be easier for maintainers to read, though it does get a bit verbose.

It might be better to just put this in a script and run the script though, then we can have sane comments and run it in other places.

@brendandburns
Copy link
Contributor

@drubin do we need to do anything more here or is this ready to merge?

@drubin
Copy link
Contributor Author

drubin commented Feb 20, 2019

@brendanburns It wasn't actually failing CI :/ I need to look into it a bit more when I have some time over the weekend, something about how travis handles these error codes and the node not returning them correctly

If you want we can close this and reopen when it's working?

@drubin drubin force-pushed the version-check-ci branch from 95c18f6 to 88f9844 Compare March 8, 2019 14:55
@k8s-ci-robot k8s-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 Mar 8, 2019
@drubin drubin force-pushed the version-check-ci branch from 88f9844 to 5ce7b43 Compare March 8, 2019 14:58
@drubin drubin changed the title WIP: add version check to ci add version check to ci Mar 8, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 8, 2019
@drubin
Copy link
Contributor Author

drubin commented Mar 8, 2019

/assign @brendanburns

This seems to work

Failing job (i manually made the version not match) https://travis-ci.org/kubernetes-client/javascript/jobs/503636171

@k8s-ci-robot
Copy link
Contributor

@drubin: GitHub didn't allow me to assign the following users: brendanburns.

Note that only kubernetes-client members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @brendanburns

This seems to work

Failing job (i manually made the version not match) https://travis-ci.org/kubernetes-client/javascript/jobs/503636171

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.

@drubin drubin force-pushed the version-check-ci branch from 5ce7b43 to e3f423e Compare March 8, 2019 15:01
@drubin
Copy link
Contributor Author

drubin commented Mar 8, 2019

/assign @brendandburns

Oops

@brendandburns
Copy link
Contributor

Nice!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 13, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, drubin

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 13, 2019
@k8s-ci-robot k8s-ci-robot merged commit b884ec2 into kubernetes-client:master Mar 13, 2019
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", 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
Development

Successfully merging this pull request may close these issues.

4 participants