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

fix: add missing prometheus config to AutomationConfig #243

Merged

Conversation

42esoulard
Copy link
Contributor

@42esoulard 42esoulard commented Oct 14, 2024

Proposed changes

Current behaviour:
Prometheus configuration is missing from the AutomationConfig when we get it via the opsmngr GetConfig call. We can see it is missing from the AutomationConfig struct. However, prometheus configuration is present when we get it directly from the opsmanager API.
Proposed behaviour:
Prometheus configuration will be properly handled and returned in opsmngr GetConfig and UpdateConfig calls.

Checklist

  • I have signed the MongoDB CLA
  • I have added tests that prove my fix is effective or that my feature works
  • I have added any necessary documentation (if appropriate)
  • I have run make fmt and formatted my code

Copy link
Collaborator

@danielblanco96 danielblanco96 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I tested it and it works well, left you just one comment.

opsmngr/automation_config.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@danielblanco96 danielblanco96 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the change 🙌

@42esoulard
Copy link
Contributor Author

I'm still testing out this latest version on my side this morning, will close this comment once I can confirm no issue was encountered

@danielblanco96
Copy link
Collaborator

@42esoulard thank you, feel free to merge it after that. I tested it myself and it looks good.

After it gets merged I'll create a new version of the library.

@42esoulard
Copy link
Contributor Author

All good on my side, sorry about the slight delay. Looks like I do not have sufficient permissions to merge myself, can you proceed with the merge? Thank you

@danielblanco96 danielblanco96 merged commit d83508c into mongodb:master Oct 17, 2024
3 checks passed
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