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.

Sign up for GitHub

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

Jump to bottom

Support Applying Configuration and Get Configuration #269

Merged
merged 27 commits into from
Nov 23, 2023
Merged

Support Applying Configuration and Get Configuration #269

merged 27 commits into from
Nov 23, 2023

Conversation

sammychinedu2ky
Copy link
Contributor

Fixes #268

@sammychinedu2ky sammychinedu2ky marked this pull request as ready for review November 19, 2023 10:23
marcin-krystianc
marcin-krystianc reviewed Nov 21, 2023
View reviewed changes
Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

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

I think it would make a lot of sense if you implement https://developer.hashicorp.com/consul/api-docs/config#get-configuration, so we can use it in the test to assert that the configuration has been applied.

@sammychinedu2ky sammychinedu2ky changed the title Support Applying Configuration Support Applying Configuration and Get Configuration Nov 21, 2023
@sammychinedu2ky
Copy link
Contributor Author

Done @marcin-krystianc

@sammychinedu2ky
Copy link
Contributor Author

Hi @marcin-krystianc. It has been resolved 🚀

marcin-krystianc
marcin-krystianc reviewed Nov 21, 2023
View reviewed changes
@sammychinedu2ky
Copy link
Contributor Author

resolved @marcin-krystianc

marcin-krystianc
marcin-krystianc reviewed Nov 23, 2023
View reviewed changes
Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

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

Looks good, just some minor comments about code duplication.

@sammychinedu2ky
Copy link
Contributor Author

Hi @marcin-krystianc . for some reason I don't know why the agentmetric code fails

@sammychinedu2ky
Copy link
Contributor Author

sammychinedu2ky commented Nov 23, 2023 •
edited
Loading

For some reason no issues in this run 😅 @marcin-krystianc

@marcin-krystianc
Copy link
Contributor

For some reason no issues in this run 😅 @marcin-krystianc

yeah, there is some flakiness in the agen-metric tests. We will observe it and if it happens too often it will need to be fixed.

@marcin-krystianc marcin-krystianc merged commit 6a62fb5 into G-Research:master Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcin-krystianc marcin-krystianc marcin-krystianc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Applying Configuration and Get Configuration
3 participants
@sammychinedu2ky @marcin-krystianc @octocat