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.

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

Rename _vagrant to vagrant #67

Merged
merged 1 commit into from
Mar 19, 2021
Merged

Rename _vagrant to vagrant #67

merged 1 commit into from
Mar 19, 2021

Conversation

gianarb
Copy link
Contributor

@gianarb gianarb commented Mar 18, 2021

Apparently the idea to prefix a package with an underscore is not that
smart as I thought. Yes go test does not run it by default when you
run go test ./... but also other commands like go mod tidy do not
work consistently.

Nothing changes in practice. By default only unit tests run. Setting the
new environment variable: TEST_WITH_VAGRANT you include the test who
uses vagrant.

@@ -1,208 +0,0 @@
package vagrant_test
Copy link
Contributor

Choose a reason for hiding this comment

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

did you forget to commit the new dir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

did I?! Maybe!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!! :D Thanks

Apparently the idea to prefix a package with an underscore is not that
smart as I thought. Yes `go test` does not run it by default when you
run `go test ./...` but also other commands like `go mod tidy` do not
work consistently.

Nothing changes in practice. By default only unit tests run. Setting the
new environment variable: `TEST_WITH_VAGRANT` you include the test who
uses vagrant.

Signed-off-by: Gianluca Arbezzano <gianarb92@gmail.com>
@@ -16,6 +17,11 @@ import (
)

func TestVagrantSetupGuide(t *testing.T) {
_, ok := os.LookupEnv("TEST_WITH_VAGRANT")
Copy link
Contributor

Choose a reason for hiding this comment

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

can also add a build constraint right? I believe that works for test files too. Not sure if thats any better than this env look up though tbh.

@gianarb gianarb removed the request for review from gauravgahlot March 18, 2021 14:13
@mmlb
Copy link
Contributor

mmlb commented Mar 18, 2021

@gianarb I imagine you want to run the vagrant action in this PR too right?

@gianarb
Copy link
Contributor Author

gianarb commented Mar 18, 2021

yes but for some reason the runner is not up, so I am looking at it first. But yes! I will merge only when vagrant on packet workflow is happy as well

@mmlb mmlb added the ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ label Mar 18, 2021
@mmlb
Copy link
Contributor

mmlb commented Mar 18, 2021

Hmm I added the label to see if GH will then block the PR until the runner is back and tests pass.

@mmlb
Copy link
Contributor

mmlb commented Mar 18, 2021

Yep looks like it does, now you can't hit merge by mistake :p

@mmlb
Copy link
Contributor

mmlb commented Mar 18, 2021

Also curious to see how this ends up failing.

@gianarb gianarb added ready-to-merge Signal to Mergify to merge the PR. and removed ready-to-merge Signal to Mergify to merge the PR. labels Mar 18, 2021
@gianarb
Copy link
Contributor Author

gianarb commented Mar 19, 2021

I am merting it manually because Mergify can't merge PRs who changes workflows

@gianarb gianarb merged commit 1a03f3b into master Mar 19, 2021
@gianarb gianarb deleted the underscore_vagrant_pkg branch March 19, 2021 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants