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

Read custom cloud list from json string #141

Merged
merged 6 commits into from
May 17, 2024

Conversation

JonCole
Copy link
Contributor

@JonCole JonCole commented May 10, 2024

This brings in the ability to specify the list of clouds from a json string, typically brought in through the grafana ini file. A seperate PR on grafana/grafana will add the ini file settings.

dependabot bot and others added 2 commits May 9, 2024 11:30
Bumps [github.com/grafana/grafana-plugin-sdk-go](https://github.com/grafana/grafana-plugin-sdk-go) from 0.219.0 to 0.230.0.
- [Release notes](https://github.com/grafana/grafana-plugin-sdk-go/releases)
- [Commits](grafana/grafana-plugin-sdk-go@v0.219.0...v0.230.0)

---
updated-dependencies:
- dependency-name: github.com/grafana/grafana-plugin-sdk-go
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
@JonCole JonCole requested a review from a team as a code owner May 10, 2024 20:53
@CLAassistant
Copy link

CLAassistant commented May 10, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@@ -22,6 +22,8 @@ type AzureSettings struct {

// This field determines which plugins will receive the settings via plugin context
ForwardSettingsPlugins []string

customClouds []*AzureCloudSettings
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized we may need to export this customClouds field after all - I was trying to keep it private to the package to push callers to use the functions, but inside grafana/grafana, we need to write this list back out as a json string when we push settings over to plugins via env variables. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be fine to remain private for now. In grafana/grafana we can still call the Clouds or CustomClouds functions to retrieve the values.

Copy link
Contributor Author

@JonCole JonCole May 13, 2024

Choose a reason for hiding this comment

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

Clouds() and CustomClouds() return AzureCloudInfo struct, which does not have all the fields that are on AzureCloudSettings struct. grafana/grafana sometimes needs the extra info on AzureCloudSettings. I think we might eventually want to get rid of the AzureCloudInfo and just return the full set of fields in one struct. Callers can use the same set of methods for all use cases. If a caller doesn't need the additional fields, they can ingore them. Thoughts? For now, I was just leaning toward exporting customClouds. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with exporting the field for now, there's nothing that would be considered a secret in there afaict

Copy link
Contributor

@aangelisc aangelisc left a comment

Choose a reason for hiding this comment

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

This LGTM, just one nit in the comments. Also, what's triggered the dependency updates?

@JonCole
Copy link
Contributor Author

JonCole commented May 13, 2024

This LGTM, just one nit in the comments. Also, what's triggered the dependency updates?

TBH - I didn't notice exactly what is bringing in the dependency updates - I will try to clean those up if I can and still get it to build.

@JonCole
Copy link
Contributor Author

JonCole commented May 14, 2024

This LGTM, just one nit in the comments. Also, what's triggered the dependency updates?

TBH - I didn't notice exactly what is bringing in the dependency updates - I will try to clean those up if I can and still get it to build.

Looks like dependabot did the dependency updates, not me - see commit: https://github.com/grafana/grafana-azure-sdk-go/pull/141/commits

@aangelisc aangelisc merged commit c271284 into grafana:main May 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.

3 participants