added `TINKERBELL_SKIP_NETWORKING` to `./generate_env.sh`

JamesPGriffith commented 3 years ago

Description

The allows vagrant up provisioner to complete.

Why is this needed

Without this change, the provisioner VM fails to complete.

Fixes: #91

How Has This Been Tested?

Executing vagrant up provisioner with the variable as an empty string:

[[TRUNCATED]]
    provisioner: ++ export TINKERBELL_SKIP_NETWORKING=
    provisioner: ++ TINKERBELL_SKIP_NETWORKING=
    provisioner: + make_certs_writable
[[TRUNCATED]]
    provisioner: ++ export TINKERBELL_SKIP_NETWORKING=
    provisioner: ++ TINKERBELL_SKIP_NETWORKING=
    provisioner: + [[ -z '' ]]
    provisioner: + setup_networking ubuntu 18.04
[[TRUNCATED]]

Removed the .env file and populate the variable string in the ./generate-env.sh and rebuilt:

[[TRUNCATED]
    provisioner: ++ export TINKERBELL_SKIP_NETWORKING=true
    provisioner: ++ TINKERBELL_SKIP_NETWORKING=true
[[TRUNCATED]
    provisioner: + [[ -z true ]]
    # IT WAS SKIPPED
    provisioner: + setup_osie
[[TRUNCATED]

This failed with running in Vagrant as expected but provides the skip behavior desired.

Note: I am not able to test this on bare-metal for which the option was originally created.

How are existing users impacted? What migration steps/scripts do we need?

Existing users are likely not impacted unless the .env was removed and recreated after #88 was merged. I was pondering whether or not the .generate_env.sh file should remove and recreate the .env file on each execution.

Checklist:

I have:

[ No ] updated the documentation and/or roadmap (if required)
However, the new option appears to need to be added to the 'On Bare Metal with Docker' section of the documentation.
[ No ] added unit or e2e tests
I honestly don't know enough Go to contribute a test for Vagrant completion.
[ No ] provided instructions on how to upgrade
There's nothing to upgrade beyond a git pull.

markjacksonfishing commented 3 years ago

Once review is complete, please remove do not merge label

dadrus commented 2 years ago

Is it really the way to go? I mean the generate_env.sh is supposed to be used for local docker setup as well. According to my understanding, the given change might break that. What about just adding the required variable to the Vagrant file like this?

provisioner.vm.provision :shell,
                        path: './scripts/tinkerbell.sh',
                        env: {
                          'TINKERBELL_CONFIGURE_NAT': configure_nat,
                          'TINKERBELL_SKIP_NETWORKING': ""
                        }

The change is then local to the vagrant setup only.

damdo commented 2 years ago

What's you opinion on this last comment Marky? :)

tstromberg commented 2 years ago

PR looks great. Can you merge this up to master and run shfmt on generate_env.sh? The linter shows a diff.

jacobweinstock commented 2 years ago

Hey @JamesPGriffith, #90 just landed and has changed the sandbox significantly. Would you mind having a look at the changes and confirm if this change is still something you'd like to see?

splaspood commented 2 years ago

In light of @jacobweinstock 's comment and its age I'm going to move that we close this PR without merge at this time.

jacobweinstock commented 2 years ago

Please do re-open this if necessary.

tinkerbell / playground