-
Notifications
You must be signed in to change notification settings - Fork 8
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
CSS-6973 Create juju jaas snap #1149
CSS-6973 Create juju jaas snap #1149
Conversation
Move service account cli package to jaas
c04414f
to
7e4dfc3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if the juju team is OK with using the jaas-plugin
vs a more generic plugin
slot content
I've asked in the other PR just now whether it should be generic or not. |
description: Juju plugin for providing JAAS functionality to the Juju CLI. | ||
version: git | ||
grade: stable | ||
base: bare |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<3
version: git | ||
grade: stable | ||
base: bare | ||
build-base: core20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not core22?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Juju snap is on core20 and my machine cannot (for some strange reason) build core22 snaps. But more the former reason, though we don't need to be on the same core anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we've renamed the CLI to juju jaas
, we should also update the docs for individual commands. We can do it here or in another PR. Up to you.
set -e | ||
CGO_ENABLED=0 go build -o juju-jaas github.com/canonical/jimm/cmd/jaas | ||
mkdir -p $GOBIN | ||
cp ./juju-jaas $GOBIN/juju-jaas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just copy to $SNAP/bin
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure $SNAP/bin
is the right location. Our other snapcraft.yaml does go install
which installs the file to $GOBIN
but that does not let you rename the binary, so I instead do it like this.
juju#16956 Recreates PR juju#16884 This PR introduces a plug into Juju CLI's Snap, allowing it to be connected to a JAAS snap that will be used to extend the Juju CLI with functionality for JAAS. This PR follows from a similar [PR in JAAS](canonical/jimm#1149) where we introduce the new Snap. The below is copied from that PR. >The mechanism by which it does this (extending the Juju CLI's functionality) is [Juju Plugins](https://juju.is/docs/juju/plugins) and Snapcraft's [content-interface](https://snapcraft.io/docs/content-interface). The juju-jaas snap shares a binary of the same name to a directory within the Juju Snap that is readable by the CLI. Because the name of the binary is juju-* the binary can be invoked via `juju jaas <command>`. It is still TBD whether it is okay to namespace the JAAS commands to `juju jaas` or whether all the JAAS commands should be surfaced as top-level commands under the Juju CLI. Regardless, the specifics for how JAAS commands are invoked can be easily changed after this by introducing symlinks/bash scripts into the JAAS Snap's `$SNAP/bin` directory of the form juju-<command> e.g. `juju-add-service-account`. ## Checklist <!-- If an item is not applicable, use `~strikethrough~`. --> ~- [ ] Code style: imports ordered, good names, simple structure, etc~ ~- [ ] Comments saying why design decisions were made~ ~- [ ] Go unit tests, with comments saying what you're testing~ ~- [ ] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing~ ~- [ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages~ ## QA steps Run the following commands to build the Juju snap, install the JAAS snap and test the integration. - `snapcraft` - `sudo snap install --dangerous --devmode ./<juju-snap-file>` - `sudo snap install jaas --channel=latest/candidate` - `sudo snap connect juju:jaas-plugin jaas` - `juju add-service-account --help` **Verifying that the new plug will not impede snap uploads:** After building the snap one can run `review-tools.snap-review` against it locally. The output is as follows: ``` $ review-tools.snap-review ./juju_3.4-rc2_amd64.snap Errors ------ - declaration-snap-v2:plugs_installation:config-lxd:personal-files human review required due to 'allow-installation' constraint (bool) - declaration-snap-v2:plugs_installation:dot-aws:personal-files human review required due to 'allow-installation' constraint (bool) - declaration-snap-v2:plugs_installation:dot-azure:personal-files human review required due to 'allow-installation' constraint (bool) - declaration-snap-v2:plugs_installation:dot-google:personal-files human review required due to 'allow-installation' constraint (bool) - declaration-snap-v2:plugs_installation:dot-kubernetes:personal-files human review required due to 'allow-installation' constraint (bool) - declaration-snap-v2:plugs_installation:dot-local-share-juju:personal-files human review required due to 'allow-installation' constraint (bool) - declaration-snap-v2:plugs_installation:dot-maas:personal-files human review required due to 'allow-installation' constraint (bool) - declaration-snap-v2:plugs_installation:dot-openstack:personal-files human review required due to 'allow-installation' constraint (bool) - declaration-snap-v2:plugs_installation:dot-oracle:personal-files human review required due to 'allow-installation' constraint (bool) ``` One can see that the new plug is not listed and should not require human review - cc @wallyworld on the hesitation raised in the previous PR.
Description
This PR introduces a new snap in JIMM's repository called
juju-jaas
. This snap is intended to be a plugin into the Juju CLI that would extend it's functionality for the JAAS controller.The mechanism by which it does this is Juju Plugins and Snapcraft's content-interface. The
juju-jaas
snap shares a binary of the same name to a directory within the Juju Snap that is readable by the CLI. Because the name of the binary isjuju-*
the binary can be invoked viajuju jaas <command>
. It is still TBD whether it is okay to namespace the JAAS commands tojuju jaas
or whether all the JAAS commands should be surfaced as top-level commands under the Juju CLI. Regardless, the specifics for how our commands are invoked can be easily changed after this by introducing symlinks/bash scripts into our Snaps$SNAP/bin
directory of the formjuju-<command>
e.g.juju-add-service-account
.Note that
juju-jaas
snap is built on the bare base to be as minimal as possible and the binary within is compiled with CGO_ENABLED=0 to ensure a statically linked binary which will avoid any issues if the build-base of our snap were to be different to the build-base of the Juju snap.Fixes CSS-6973
Engineering checklist
Check only items that apply
Test instructions
Introduced the appropriate plug into the Juju Snap (will make that PR next and update this). Then compiled both snaps and installed them with
sudo snap install --dangerous --devmode <snap-file>
and then ransudo snap connect juju:jaas-plugin juju-jaas
at which point thejuju-jaas
commands became available, see below: