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

CSS-10401 cleanup repo #1331

Merged
merged 7 commits into from
Aug 30, 2024
Merged

CSS-10401 cleanup repo #1331

merged 7 commits into from
Aug 30, 2024

Conversation

kian99
Copy link
Contributor

@kian99 kian99 commented Aug 28, 2024

Description

Note: This PR builds on #1330.

This PR updates JIMM's README to be more accurate to the current state of affairs. It also splits out the testing instructions into a separate CONTRIBUTING.md file.

Partially addresses CSS-10401

Engineering checklist

Check only items that apply

  • Documentation updated
  • Covered by unit tests
  • Covered by integration tests

@kian99 kian99 requested a review from a team as a code owner August 28, 2024 09:50
ale8k
ale8k previously approved these changes Aug 28, 2024
Copy link
Contributor

@ale8k ale8k left a comment

Choose a reason for hiding this comment

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

some comments but I'll approve and just re-request approval when you've done the ones you think are worth doing! Nice work!

.vscode/extensions.json Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
Subsequently JIMM introduced a large shift in how the service worked:
- JIMM now acts as a proxy between all client and Juju controller interactions. Previously
users were redirected to a Juju controller.
- Juju controllers trust a public key served by JIMM.
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be confusing, they trust JWT (insert what algo we use here) cryptography

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reoworded to just "Juju controllers support JWT login where secure tokens are issued by JIMM."

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
internal/cmdtest/cmdsetup.go Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved

At this point, from the root of this branch, run the command:
`make install`
## Dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about adding a small drawing in the readme? I can create it in another pr.

example.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a nice diagram in our JAAS docs, but I like this idea. We can add one in a follow-up.

README.md Outdated Show resolved Hide resolved

- jemd: start the JIMM server;
- jaas-admin: perform admin commands on JIMM;
A brief explanation of the various services that JIMM depends on is below:
Copy link
Contributor

Choose a reason for hiding this comment

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

i think Jimm depends also on "juju"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, but I think that's implied by the fact that JIMM acts as a layer on top of Juju. And JIMM can run without Juju present but it can't start with OpenFGA, Postgres and Vault.

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ale8k ale8k left a comment

Choose a reason for hiding this comment

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

minor little comments, but lgtm, nice!!


The project uses Go modules (https://golang.org/cmd/go/#hdr-Module_maintenance) to manage Go
dependencies. **Note: Go 1.11 or greater needed.**
JIMM/JAAS provides enterprise level functionality layered on top of your Juju controller like:
Copy link
Contributor

Choose a reason for hiding this comment

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

functionalities? And I'd probably just remove the "like" bit

- jimmsrv: The JIMM server.
- jimmctl: A CLI tool for administrators of JIMM to view audit logs, manage permissions, etc.
Available as a snap.
- jaas: A plugin for the Juju CLI, extend the base set of command with extra functionality when
Copy link
Contributor

Choose a reason for hiding this comment

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

extends* with additional* ?


Further, to better align the two projects, JIMM's versioning now aligns with Juju.

As a result of this, there is no JIMM v2 and instead from JIMM v3, the versioning strategy
Copy link
Contributor

Choose a reason for hiding this comment

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

and instead, (needs comma)

@kian99
Copy link
Contributor Author

kian99 commented Aug 30, 2024

@ale8k Thanks, I'll fix those last 3 comments in a follow-up.

@ale8k
Copy link
Contributor

ale8k commented Aug 30, 2024

@ale8k Thanks, I'll fix those last 3 comments in a follow-up.

no worries!

@kian99 kian99 merged commit cbcf694 into canonical:v3 Aug 30, 2024
4 checks passed
kian99 added a commit to kian99/jimm that referenced this pull request Sep 3, 2024
* improve readme and supporting docs

* PR tweaks

* Add a line explaining building/publishing section

* put jimm history in a separate doc

* add project links
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.

3 participants