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

Create new script to create client credentials in MongoDB #388

Merged
merged 4 commits into from
Nov 8, 2024
Merged

Conversation

val500
Copy link
Contributor

@val500 val500 commented Oct 28, 2024

Description

This creates a new script to create, edit, or delete client credentials from MongoDB. MongoDB login credentials must be specified using environment variables. User gets prompted for the new client_id and secret_key and must input the max_priority and allowed_queues data.

Resolved issues

Resolves https://warthogs.atlassian.net/browse/CERTTF-437

Documentation

Web service API changes

N/A

Tests

Tested locally using docker compose to spin up MongoDB. Creation, editing and deletion were all tested.

Copy link
Collaborator

@plars plars left a comment

Choose a reason for hiding this comment

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

A few comments below, but a few more general comments:

  1. please run flake8/pylint/black -n79 on this
  2. There's a big problem I found - you can add the same client ID more than once. We shouldn't allow that
  3. There should be some confirmation step at the end where it shows you what it's going to add (except the secret) and ask to confirm whether to add it or not

server/tools/client_credentials_admin.py Outdated Show resolved Hide resolved
server/tools/client_credentials_admin.py Outdated Show resolved Hide resolved
server/tools/client_credentials_admin.py Outdated Show resolved Hide resolved
server/tools/client_credentials_admin.py Outdated Show resolved Hide resolved
server/tools/client_credentials_admin.py Outdated Show resolved Hide resolved
server/tools/client_credentials_admin.py Outdated Show resolved Hide resolved
server/tools/client_credentials_admin.py Outdated Show resolved Hide resolved
@val500 val500 force-pushed the CERTTF-437 branch 2 times, most recently from 5a011f7 to a50aac0 Compare November 1, 2024 10:25
@val500 val500 requested a review from plars November 1, 2024 10:29
Copy link
Collaborator

@plars plars left a comment

Choose a reason for hiding this comment

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

Found a couple more small issues, and I think there were still some earlier comments.

Additionally, I had trouble making it work if you use pip install. I know we use poetry, but we still try to import pip install for the charm, previous way of doing things, etc. And both should work fine

I did solve this with a few minor changes. I can just submit a patch against your branch if you prefer... just let me know, but might be easier for you to make the other changes first. But here are the things I needed to change to get it working:

  • move tools to src/tools
  • add a src/tools/init.py
  • pyproject.toml: add this under tool.poetry section:
+packages = [
+    { include = "src" },
+]
  • set client_credentials_admin = "src.tools.client_credentials_admin:main" (added "src" to the path)

server/tools/client_credentials_admin.py Outdated Show resolved Hide resolved
server/tools/client_credentials_admin.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@plars plars left a comment

Choose a reason for hiding this comment

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

Ok, I think this works well enough for our short term needs. It's something we'll replace with an API pretty soon I hope, but it'll get us though until then. Thanks!

@plars plars merged commit a77c0ee into main Nov 8, 2024
1 check passed
@plars plars deleted the CERTTF-437 branch November 8, 2024 17:02
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.

2 participants