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

Add composite action for starting a JIMM environment #1321

Merged
merged 8 commits into from
Aug 23, 2024

Conversation

kian99
Copy link
Contributor

@kian99 kian99 commented Aug 21, 2024

Description

This PR creates a Github "composite action", i.e. an action that can be called from other workflows for the purposes of allowing other projects to spin up JIMM as part of an integration test. As mentioned in the readme.md file for the action, the high-level steps it takes are:

  1. Starts JIMM's docker compose test environment.
  2. Uses https://github.com/charmed-kubernetes/actions-operator action to start a Juju controller and connects it to JIMM.
  3. Ensures the local Juju CLI is setup to communicate with JIMM authenticating as a test user.

The motivation for this change is to provide the Juju Terraform provider with a way of running integration tests against JIMM.

Additionally I've made 2 further changes:

  • I've adjusted our docker compose to add a jimm-test container, which is similar to the current jimm container except it does not build jimm from source and instead pulls the image from the Github Container Registry. This makes the environment much faster to start up since no Go modules need to be downloaded and nothing needs to be compiled.
  • As part of the above I moved the environment variables used for the JIMM container to a separate file in the repo so that we don't duplicate them.
  • I've added an integration-test.yaml file that uses the new composite action as part of a basic integration test. This verifies that the composite action is working and gives us a nice, easy to spin up testing environment which we could use in the future for further tests.

Partially addresses CSS-6347

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 21, 2024 13:47
- Use the composite action as part of a basic integration test
- Adding a trigger to run the new integration test on PRs to verify it works.
@kian99 kian99 force-pushed the CSS-6347-integration-testing branch from 4dff75d to 195794f Compare August 22, 2024 09:47
alesstimec
alesstimec previously approved these changes Aug 22, 2024
alesstimec
alesstimec previously approved these changes Aug 22, 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.

Think we need to move the compose around a bit.

  • 2 compose files would be better
  • .env files don't allow host interpolation and can be confusing, let's do it inline on both files

.air.toml Outdated Show resolved Hide resolved
.github/actions/test-server/action.yaml Show resolved Hide resolved
.github/actions/test-server/action.yaml Show resolved Hide resolved
uses: actions/setup-go@v4
with:
go-version-file: 'go.mod'
- name: Go vendor to speed up docker build
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would this speed it up? Wouldn't it be exactly the same as referencing our mod cache?

Copy link
Contributor Author

@kian99 kian99 Aug 22, 2024

Choose a reason for hiding this comment

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

It speeds up the build inside the docker container because we mount the working directory into the air container and with the vendor folder present, Go does not need to download all the dependencies.

It would indeed be the same thing as adding a volume mount to the mod cache, I can do that instead if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

But vendoring downloads them just like a build would? This would only make it faster when you're bring the compose up and down right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Vendoring puts them in the repo in ./vendor and they are already present in the Go module cache because of the setup-go action. So this ends up bringing them into the air docker image which then greatly speeds up the build.

docker-compose.yaml Show resolved Hide resolved
test.env 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.

meant to request changes

- Use `extends` to shift common config to a separate file
@kian99 kian99 mentioned this pull request Aug 23, 2024
@kian99 kian99 requested a review from ale8k August 23, 2024 12:43
@kian99 kian99 mentioned this pull request Aug 23, 2024
- name: Go vendor to speed up docker build
if: ${{ github.event_name == 'pull_request' }}
run: go mod vendor
- name: Start JIMM (pull request)
Copy link
Contributor

Choose a reason for hiding this comment

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

Some new lines between plz

@@ -0,0 +1,62 @@
# This file contains a collection of common configurations used in JIMM's Docker compose file.
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 I've conventionally seen another dot, like docker-compose.qa.yaml, can we do that instead for the name plz?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this file isn't a docker compose file, it's just a base template. With that in mind what do you think for the name?

# An instance of JIMM used in integration tests, pulled from a tag.
jimm-test:
extends:
file: compose-common.yaml
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 having an actual separate compose file is better under a specific network name?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://stackoverflow.com/questions/38088279/communication-between-multiple-docker-compose-projects

But we don't need external or anything, just have them in the same network

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 won't do this now because it's more complexity than we need. We're just extending some config rather than needing to create multiple composes.

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.

lgtm but I'd much prefer we just have a docker-compose.qa.yaml in idk "jimm-local-network" or something

@kian99 kian99 merged commit db18dbf into canonical:v3 Aug 23, 2024
4 checks passed
kian99 added a commit to kian99/jimm that referenced this pull request Sep 3, 2024
* Add composite action for starting a JIMM environment

- Use the composite action as part of a basic integration test

* Run integration test on PR

- Adding a trigger to run the new integration test on PRs to verify it works.

* fix naming

* allow action to start dev env

* rewording

* remove need for env file

* add default string

* use extends in compose

- Use `extends` to shift common config to a separate file
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