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

parameterize VersionsInSummary - complete fix #3703

Merged
merged 7 commits into from
Nov 4, 2021
Merged

parameterize VersionsInSummary - complete fix #3703

merged 7 commits into from
Nov 4, 2021

Conversation

satya-dillikar
Copy link
Contributor

@satya-dillikar satya-dillikar commented Oct 29, 2021

Description of the change

Users can now control the “Package Version” shown in the UX via values.yaml file. By default, versionSummary is set as { major: 3, minor: 3, patch: 3 }. But users can customize these values as per their needs.

The configured values are passed to the helm plugin via kubeapps-apis server --plugin-config-path param.

Benefits

This new parameter VersionsInSummary can be populated from the values.yaml file of the helm chart.

Possible drawbacks

None

Applicable issues

Users can now control the “Package Version” shown in the UX via values.yaml file. By default, versionSummary is set as { major: 3, minor: 3, patch: 3 }. But users can customize these values as per their needs.

Additional information

More proper fix is still pending.

Copy link
Contributor

@absoludity absoludity 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, thanks Satya. You might want to update the "Fixes xxxx." in the PR description to instead just reference the issue (ie. "Ref: xxxx"), otherwise landing this PR will close the issue as fixed.

name string
chart_versions []models.ChartVersion
version_summary []*corev1.PackageAppVersion
exp_versions_in_summary VersionsInSummary
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering what the "exp_" is for here? This one isn't an expected value, but rather an input value? The expected value is the resulting version summary? Fine either way, just confused me reading it.

@satya-dillikar satya-dillikar changed the title parameterize VersionsInSummary -PART#1 : add two new TCs parameterize VersionsInSummary - complete fix Nov 1, 2021
Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

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

Excellent focused change @satya-dillikar , well done!

Two small things worth mentioning:

  1. As per the inline comments, I'm not sure about putting the kubeapps-apis plugins config in the shared config map (it's shared only because the clusters config is re-used across services, whereas the plugins config is specific). Maybe it's fine as is, given it's only mounted if explicitly wanted?
  2. Do you think it's worth adding a test function with a couple of test cases for your new parsePluginConfig function? If so, you can use some yaml as input, the test converts the yaml to json, writes it to a temporary file (remember to delete afterwards in your defer, see an example), then calls the function with the filename to verify the expected result.
  3. We need to update the version in the Chart.yaml. In your branch it's currently set to 7.5.12-dev1 in the Chart.yaml, so just update that to 7.5.12-dev2 please.

Note, the e2e test had spuriously failed. I re-ran it and it passed.

Let me know what you think, feel free to land it when you've updated the chart version, depending whether you also add the tests.

@@ -14,4 +14,6 @@ metadata:
data:
clusters.conf: |-
{{ .Values.clusters | toPrettyJson | indent 4 }}
plugins.conf: |-
{{ .Values.kubeappsapis.pluginConfig | toPrettyJson | indent 4 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

As I mentioned earlier, not sure about including the pluginConfig here. On the plus side, it's easier to do now, but on the negative side, we're putting the very specific plugin config for the kubeapps-apis service in a shared configmap that's used for other services (it's shared here specifically because the clusters.conf is used on many services).

That said, even if other services use this configmap as a volume, the plugin configuration will only be mounted as a file on the file system if the deployment has a corresponding volumeMount for it, so perhaps it's fine as is. What do you think? Fine either way for me.

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 have created a new configmap for kubeappsapis

Comment on lines 104 to 110
Helm struct {
Packages struct {
V1alpha1 struct {
SomeConfigOption string `json:"someConfigOption"`
} `json:"v1alpha1"`
} `json:"packages"`
} `json:"helm"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This was just an example, I don't think we should include it here (or if we do, as a comment to show what can be done).

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 have removed this.

versionsInSummary VersionsInSummary
}

func parsePluginConfig(pluginConfigPath string) VersionsInSummary {
Copy link
Contributor

Choose a reason for hiding this comment

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

For now this is fine, since VersionsInSummary is the only configurable option for the plugin, but I assume in the future we'd be returning the helmConfig struct.

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 shall keep as-is for now. But updated comments in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please review the new set of changes in my new commit on Nov3
"add new TC & configmap"

I had some merge issues, hence you will see more commits. Please ignore those.
Thanks again.

@satya-dillikar
Copy link
Contributor Author

Excellent focused change @satya-dillikar , well done!

Two small things worth mentioning:

  1. As per the inline comments, I'm not sure about putting the kubeapps-apis plugins config in the shared config map (it's shared only because the clusters config is re-used across services, whereas the plugins config is specific). Maybe it's fine as is, given it's only mounted if explicitly wanted?
  2. Do you think it's worth adding a test function with a couple of test cases for your new parsePluginConfig function? If so, you can use some yaml as input, the test converts the yaml to json, writes it to a temporary file (remember to delete afterwards in your defer, see an example), then calls the function with the filename to verify the expected result.
  3. We need to update the version in the Chart.yaml. In your branch it's currently set to 7.5.12-dev1 in the Chart.yaml, so just update that to 7.5.12-dev2 please.

Note, the e2e test had spuriously failed. I re-ran it and it passed.

Let me know what you think, feel free to land it when you've updated the chart version, depending whether you also add the tests.

I have addressed all three points mentioned here in my below commit.

Copy link
Contributor

@absoludity absoludity 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 update. A couple of small thoughts that you can ignore if you want, just see what you think.

Land when ready (do you have permission to merge the PR? otherwise let me know and I'll do it when you're ready).

@satya-dillikar
Copy link
Contributor Author

Thanks for the update. A couple of small thoughts that you can ignore if you want, just see what you think.

Land when ready (do you have permission to merge the PR? otherwise let me know and I'll do it when you're ready).

I don't see the option to "merge in this PR". Probably I don't have permission. Please merge or add me with the permissions.

Thanks

@absoludity absoludity merged commit 9700927 into vmware-tanzu:master Nov 4, 2021
@satya-dillikar satya-dillikar deleted the versionInSummary_part1 branch November 4, 2021 16:38
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.

Some Chart versions hidden from dropdown when deploying an application
2 participants