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

Extra setup details #1089

Merged
merged 3 commits into from
Nov 16, 2023
Merged

Extra setup details #1089

merged 3 commits into from
Nov 16, 2023

Conversation

huwshimi
Copy link
Contributor

Description

Added some additional set up instructions for developers.

Comment on lines +21 to +23
After this initial setup, subsequent use of the compose can be done with `docker
compose --profile dev up --force-recreate` (it may be necessary to run `docker
compose down -v --remove-orphans` first).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there is a better way to explain this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you looking for an explanation of the extra flags? Or just a rewording to make it clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if we should explain why it's necessary to run, or what error to look out for.

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 it's okay like this, a quick google would reveal a better answer likely anyway

local/README.md Outdated
1. The following commands might need to be run to work around an [LXC networking
issue](https://github.com/docker/for-linux/issues/103#issuecomment-383607773):
`sudo iptables -F FORWARD && sudo iptables -P FORWARD ACCEPT`.
2. Install Juju: `sudo snap install 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.

Should this specify a minimum Juju version?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes in fact, this version of JIMM with OpenFGA requires Juju 3.3 and up (though it also works with Juju 3.2 but officially 3.3 is the first supported version).

local/README.md Outdated
1. The following commands might need to be run to work around an [LXC networking
issue](https://github.com/docker/for-linux/issues/103#issuecomment-383607773):
`sudo iptables -F FORWARD && sudo iptables -P FORWARD ACCEPT`.
2. Install Juju: `sudo snap install juju`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes in fact, this version of JIMM with OpenFGA requires Juju 3.3 and up (though it also works with Juju 3.2 but officially 3.3 is the first supported version).

Comment on lines +21 to +23
After this initial setup, subsequent use of the compose can be done with `docker
compose --profile dev up --force-recreate` (it may be necessary to run `docker
compose down -v --remove-orphans` first).
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you looking for an explanation of the extra flags? Or just a rewording to make it clearer?

local/README.md Outdated
10. `docker compose --profile dev up` if you encounter an error like "Error response from daemon: network ... not found" then the command `docker compose --profile dev up --force-recreate` should help.

After this initial setup, subsequent use of the compose can be done with `docker compose --profile dev up --force-recreate`
5. Ensure you are in the root JIMM directory.
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 the numbering skips 4.

Copy link
Collaborator

@alesstimec alesstimec left a comment

Choose a reason for hiding this comment

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

LGTM once the numbering issue is resolved and the juju version is fixed to 3.3

@kian99 kian99 merged commit 803c906 into canonical:v3 Nov 16, 2023
2 checks passed
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