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

Relate to keystone to get credentials for exporter #4

Merged
merged 3 commits into from
Mar 21, 2024

Conversation

samuelallan72
Copy link
Contributor

@samuelallan72 samuelallan72 commented Mar 18, 2024

Save these credentials to a clouds.yaml file,
so they can be used by openstack-exporter.

If keystone in your test environment uses https, you may need to manually set the ssl_ca config.

internal ticket: BSENG-2172

@samuelallan72 samuelallan72 self-assigned this Mar 18, 2024
@samuelallan72 samuelallan72 requested a review from a team as a code owner March 18, 2024 00:04
@samuelallan72 samuelallan72 changed the title WIP: Integrate with keystone to get admin credentials Relate to keystone to get credentials for exporter Mar 19, 2024
@samuelallan72 samuelallan72 force-pushed the add-keystone branch 2 times, most recently from 3c33551 to 6aa7c31 Compare March 19, 2024 04:15
Copy link
Contributor

@chanchiwai-ray chanchiwai-ray left a comment

Choose a reason for hiding this comment

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

Small issues and some questions

README.md Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
@samuelallan72
Copy link
Contributor Author

@chanchiwai-ray thanks for your review! I responded to your comments; please see latest commits. :)

README.md Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
@samuelallan72
Copy link
Contributor Author

Thanks @jneo8 and @chanchiwai-ray , I responded to your comments.

src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
chanchiwai-ray
chanchiwai-ray previously approved these changes Mar 20, 2024
Copy link
Contributor

@chanchiwai-ray chanchiwai-ray left a comment

Choose a reason for hiding this comment

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

But overall it looks good to me. I don't want to be picky right now and block the PR here

jneo8
jneo8 previously approved these changes Mar 20, 2024
@samuelallan72 samuelallan72 marked this pull request as draft March 20, 2024 02:43
@samuelallan72 samuelallan72 dismissed stale reviews from jneo8 and chanchiwai-ray via 8a6a474 March 20, 2024 02:43
@samuelallan72 samuelallan72 force-pushed the add-keystone branch 3 times, most recently from f7e62f3 to 2ce3eb7 Compare March 20, 2024 02:57
Save these credentials to the clouds.yaml file,
so they can be used by openstack-exporter.
@samuelallan72 samuelallan72 marked this pull request as ready for review March 20, 2024 03:55
chanchiwai-ray
chanchiwai-ray previously approved these changes Mar 20, 2024
Copy link
Contributor

@chanchiwai-ray chanchiwai-ray left a comment

Choose a reason for hiding this comment

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

Thanks, overall looks good, I left a comment about snap_service.restart().

(optional and opinionated) Can we make the non-event function without the _ prefix? It just looks nicer to me, and make me easier to visually distinguish the event callbacks and normal functions

src/charm.py Outdated Show resolved Hide resolved
@samuelallan72
Copy link
Contributor Author

Can we make the non-event function without the _ prefix? It just looks nicer to me, and make me easier to visually distinguish the event callbacks and normal functions

So ideally all the other methods should have the _ prefix too, as this is used as a convention to show that they are internal methods and shouldn't be called from outside the class.

@chanchiwai-ray
Copy link
Contributor

Can we make the non-event function without the _ prefix? It just looks nicer to me, and make me easier to visually distinguish the event callbacks and normal functions

So ideally all the other methods should have the _ prefix too, as this is used as a convention to show that they are internal methods and shouldn't be called from outside the class.

Yes, I understand that, so it's optional

Copy link
Contributor

@chanchiwai-ray chanchiwai-ray left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

Did some basic testing with keystone:

  • installed with resource -> Blocked: missing keystone relation
  • relate to keystone -> Active
  • curl the endpoint -> Blocked: snap service is not running (after update-status) (note this is expected with only keystone)
  • changing config ssl_ca render the clouds.yaml

@samuelallan72 samuelallan72 merged commit 9cc63c6 into main Mar 21, 2024
@samuelallan72 samuelallan72 deleted the add-keystone branch March 21, 2024 04:21
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