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

feat: Remove apt-install from Dex Charm #148

Merged
merged 1 commit into from
Aug 4, 2023
Merged

Conversation

phoevos
Copy link
Contributor

@phoevos phoevos commented Aug 3, 2023

Currently, Dex Charm is doing apt install. Update this logic to ensure the Dex Charm can work in an air-gapped environment

Closes #100

@phoevos phoevos requested a review from a team as a code owner August 3, 2023 08:52
beliaev-maksim
beliaev-maksim previously approved these changes Aug 3, 2023
@kimwnasptd
Copy link
Contributor

@beliaev-maksim you approved the PR within 4 mins after it was created, and even before the tests were finished.

When did you have the time to test and be sure the PR is OK? 😄

@beliaev-maksim
Copy link
Member

beliaev-maksim commented Aug 3, 2023

@kimwnasptd PR changes are fine. If CI is red then it automatically assumes that it should not be merged :)
since not all CI yet executed, you can merge only using "admin" power. The same you can do even without approval :D

beliaev-maksim
beliaev-maksim previously approved these changes Aug 3, 2023
@phoevos
Copy link
Contributor Author

phoevos commented Aug 3, 2023

I believe this is what's causing the integration tests to fail:

WARNING  selenium.webdriver.common.selenium_manager:selenium_manager.py:133 The chromedriver version (115.0.5790.110) detected in PATH at /usr/bin/chromedriver might not be compatible with the detected chrome version (115.0.5790.170); currently, chromedriver 115.0.5790.170 is recommended for chrome 115.*, so it is advised to delete the driver in PATH and retry

Because then this is the exception raised:

selenium.common.exceptions.WebDriverException: Message: unknown error: no chrome binary at /opt/google/chrome/google-chrome

@beliaev-maksim
Copy link
Member

@phoevos switch to firefox

@phoevos
Copy link
Contributor Author

phoevos commented Aug 3, 2023

I opened an issue for switching to Firefox, I'm not sure at the moment how much effort that requires:

Currently, Dex Charm is doing apt install. Update this logic to ensure
the Dex Charm can work in an airgapped environment.

Signed-off-by: Phoevos Kalemkeris <phoevos.kalemkeris@canonical.com>
@phoevos
Copy link
Contributor Author

phoevos commented Aug 4, 2023

Looks like the webdriver issue resolved itself, so this is again up for review🎉

@phoevos phoevos merged commit 4e42ce3 into main Aug 4, 2023
@phoevos phoevos deleted the kf-3993-drop-apt-install branch August 4, 2023 14:01
@kimwnasptd
Copy link
Contributor

@phoevos @beliaev-maksim to confirm. Do we verify now that the Charm works in airgapped environment since it doesn't do apt-install? Did you try this as well as part of the review?

@beliaev-maksim
Copy link
Member

@kimwnasptd I believe airgapped should be tested as separate task, since there could be other pieces that will not work

@orfeas-k orfeas-k restored the kf-3993-drop-apt-install branch August 7, 2023 12:27
@orfeas-k orfeas-k deleted the kf-3993-drop-apt-install branch August 7, 2023 12:27
DnPlas pushed a commit that referenced this pull request Sep 26, 2023
Currently, Dex Charm is doing apt install. Update this logic to ensure
the Dex Charm can work in an airgapped environment.

Closes #100

Signed-off-by: Phoevos Kalemkeris <phoevos.kalemkeris@canonical.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove installation of bcrypt in Charm code
3 participants