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

Eliminate the need for TLS certs in tests #1330

Merged
merged 3 commits into from
Aug 28, 2024
Merged

Conversation

kian99
Copy link
Contributor

@kian99 kian99 commented Aug 28, 2024

Description

This PR modifies the jimmctl and jaas cmd tests to eliminate the need to setup the JIMM server with TLS certs.
Although the Juju API client hardcodes its connection to the controller with wss:// to enforce secure websockets, it does provide the ability to inject a custom websocket dialer.

In our tests, we provide a custom websocket dialer where we replace the wss:// in the host address with ws:// to allow JIMM to be run as a plain HTTP server.

I've also updated our CI workflow to remove the step where it was setting up certs and additionally removed the environment variables we were setting that were not needed.

Big thanks to @ale8k for the suggestion on using a custom dialer.

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:21
@kian99 kian99 mentioned this pull request Aug 28, 2024
3 tasks
ale8k
ale8k previously approved these changes Aug 28, 2024
internal/cmdtest/cmdsetup.go Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Show resolved Hide resolved
.github/workflows/ci.yaml Show resolved Hide resolved
internal/cmdtest/cmdsetup.go Show resolved Hide resolved
@kian99 kian99 merged commit 98c95bf into canonical:v3 Aug 28, 2024
4 checks passed
kian99 added a commit to kian99/jimm that referenced this pull request Sep 3, 2024
* eliminate the need for TLS certs in tests

* remove InsecureSkipVerify config
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.

4 participants