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

No inputs disables snap install #5

Merged
merged 1 commit into from
Jan 18, 2023
Merged

No inputs disables snap install #5

merged 1 commit into from
Jan 18, 2023

Conversation

barrettj12
Copy link
Collaborator

@barrettj12 barrettj12 commented Jan 16, 2023

If this action is invoked with no channel input, it will not attempt to refresh LXD from snap - so you can use the pre-installed LXD version.

Also added some more GitHub tests.

@barrettj12
Copy link
Collaborator Author

On second thought, maybe channel: '' (or unset) should be reserved for the default channel (IINM this can be different from latest/stable?), and we should have an explicit disable-snap-install input.

Copy link

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

I personally prefer the explicit configuration options (e.g. disable-snap-install) and not a magic values.
On a long run it always makes easier to deprecate the option, etc.
Deprecating the value is a way harder.

Also, essentially user might expect identical behavior for:

    - name: Setup LXD
      uses: canonical/setup-lxd@[version]
      with:
        channel: ''

and

    - name: Setup LXD
      uses: canonical/setup-lxd@[version]

While they are dramatically different. It can lead to bad surprises for the end-users.
You are the author, the PR will work for me, I will obey. LGTM.

PR#4 is updated, I close it in favor of this PR.

@barrettj12
Copy link
Collaborator Author

Yeah, I think I agree that an explicit input is better. I'll change my PR to match.

Copy link
Member

@jnsgruk jnsgruk left a comment

Choose a reason for hiding this comment

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

One final change here then I think we're good to go:

If channel is unset, we still need to check if LXD is installed, and install with snap install lxd if it isn't. I don't like the idea that canonical/setup-lxd could fail due to not installing LXD ;-)

- add tests
- fix juju integration test
@barrettj12 barrettj12 changed the title channel: '' disables snap install No inputs disables snap install Jan 18, 2023
@barrettj12 barrettj12 merged commit 8e55622 into main Jan 18, 2023
@barrettj12 barrettj12 deleted the disable-snap branch January 18, 2023 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants