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.

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

vagrant: Improve command-line usability #10933

Merged
merged 3 commits into from Apr 15, 2020
Merged

vagrant: Improve command-line usability #10933

merged 3 commits into from Apr 15, 2020

Conversation

pchaigno
Copy link
Member

@pchaigno pchaigno commented Apr 10, 2020 •
edited

Please see commit messages.

@pchaigno pchaigno added pending-review area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. release-note/misc This PR makes changes that have no direct user impact. labels Apr 10, 2020
@pchaigno pchaigno requested a review from a team as a code owner April 10, 2020 13:56
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Apr 10, 2020
@pchaigno pchaigno changed the title vagrant: Improve command line usability vagrant: Improve command-line usability Apr 10, 2020
pchaigno
pchaigno commented Apr 10, 2020
View reviewed changes
Vagrantfile
@@ -13,6 +13,7 @@ $NETNEXT_SERVER_VERSION= (ENV['NETNEXT_SERVER_VERSION'] || $NETNEXT_SERVER_VERSI
if ENV['NETNEXT'] == "true"
$SERVER_BOX = $NETNEXT_SERVER_BOX
$SERVER_VERSION = $NETNEXT_SERVER_VERSION
$vm_kernel = '+'
Copy link
Member Author

@pchaigno pchaigno Apr 10, 2020 •
edited

Choose a reason for hiding this comment

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

For reviewers: Not sure if this is clear enough, but I wanted to keep the VM names as short as possible, since vagrant command-line completion is often buggy.

@coveralls
Copy link

coveralls commented Apr 10, 2020 •
edited

Coverage Status

Coverage increased (+0.02%) to 46.663% when pulling 94ed694 on pr/pchaigno/improve-vagrant-commandline into 1000f70 on master.

@pchaigno pchaigno marked this pull request as draft April 10, 2020 15:34
@pchaigno pchaigno marked this pull request as ready for review April 10, 2020 19:01
@pchaigno pchaigno requested a review from a team as a code owner April 10, 2020 19:01
aanm
aanm requested changes Apr 14, 2020
View reviewed changes
Vagrantfile Show resolved Hide resolved
@pchaigno pchaigno force-pushed the pr/pchaigno/improve-vagrant-commandline branch from 7d4be7e to 1adcdf4 Compare April 14, 2020 10:58
qmonnet
qmonnet approved these changes Apr 14, 2020
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment •
edited

Choose a reason for hiding this comment

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

Looks good to me (besides the + thing), and looks helpful!

Should we change the jenkinsfiles too to NETNEXT=1 so we can deprecate NETNEXT=true in the future, or would that risk to break the CI?

@pchaigno
vagrant: Distinct name for net-next VMs
b116dec 
Having a distinct name for net-next VMs allows us to switch between
NETNEXT=true and NETNEXT=false without destroying VMs each time. It
should also be possible to run both VMs at the same time.

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno
vagrant: Do not require env. varables for all Vagrant cmds
dd265d4 
Currently, to e.g. destroy a Vagrant VM, it is necessary to set some of
the same options that were used to create it (e.g., K8S, NWORKERS, and
now with the previous commit, NETNEXT). This commit fixes this by always
defining common VM names, so that commands like 'vagrant destroy' or
'vagrant ssh' work without setting env. variables.

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno
vagrant, test: Accept NETNEXT=1 syntax
94ed694 
Document NETNEXT option for the dev. VM and accept NETNEXT=1 syntax for
consistency with other options.

Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno force-pushed the pr/pchaigno/improve-vagrant-commandline branch from 1adcdf4 to 94ed694 Compare April 15, 2020 07:40
@pchaigno
Copy link
Member Author

Should we change the jenkinsfiles too to NETNEXT=1 so we can deprecate NETNEXT=true in the future, or would that risk to break the CI?

Yep, that makes sense. I've switched Jenkinsfiles usage to NETNEXT=1 as well.

Note, however, that even once NETNEXT=true is deprecated, we won't have the same behavior as for other options: other options are enabled/disabled if the corresponding macro is defined/undefined; the macro's value doesn't matter. Conversely, for NETNEXT, because of how it's used by CI, the value does matter. We would need to update our Jenkins library to change that.

@qmonnet
Copy link
Member

qmonnet commented Apr 15, 2020

Note, however, that even once NETNEXT=true is deprecated, we won't have the same behavior as for other options: other options are enabled/disabled if the corresponding macro is defined/undefined; the macro's value doesn't matter. Conversely, for NETNEXT, because of how it's used by CI, the value does matter. We would need to update our Jenkins library to change that.

😲 Ok, thanks for the explanation. I suppose consistency from user point-of-view is a good thing already, no point in changing the Jenkins lib if it's not broken. Thanks for the changes, looks good to me!

@pchaigno
Copy link
Member Author

restart-ginkgo

@vadorovsky vadorovsky merged commit 263fbd5 into master Apr 15, 2020
1.8.0 automation moved this from In progress to Merged Apr 15, 2020
@vadorovsky vadorovsky deleted the pr/pchaigno/improve-vagrant-commandline branch April 15, 2020 13:07
@qmonnet qmonnet mentioned this pull request Mar 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanm aanm aanm approved these changes

@qmonnet qmonnet qmonnet approved these changes

@vadorovsky vadorovsky vadorovsky approved these changes

Assignees
No one assigned
Labels
area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pchaigno @coveralls @qmonnet @aanm @vadorovsky