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

Add configurable hints for machine image vendors #1066

Merged
merged 8 commits into from
Jul 30, 2021

Conversation

grolu
Copy link
Contributor

@grolu grolu commented Jul 22, 2021

What this PR does / why we need it:
Some vendors require additional hints, e.g. to inform the user that an enterprise license is required for usage.
With this PR it is possible to define those hints via the dashboard configuration.

Capture

Example:

apiVersion: v1
kind: ConfigMap
data:
  config.yaml: |
    ---
    frontend:
      vendorHints:
        - matchNames:
            - suse-chost
            - suse-jeos
          message: Enterprise license required
          severity: warning

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

It is now possible to add configurable hints for machine image vendors

grolu added 2 commits July 22, 2021 11:52
# Conflicts:
#	frontend/src/store/index.js
#	frontend/tests/unit/store.index.spec.js
@gardener-robot gardener-robot added needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Jul 22, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 22, 2021
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 22, 2021
petersutter
petersutter previously approved these changes Jul 22, 2021
Copy link
Member

@petersutter petersutter left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Jul 22, 2021
{{- range .vendorNames }}
- {{ . }}
{{- end }}
hintMessage: {{ .hintMessage }}
Copy link
Member

Choose a reason for hiding this comment

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

May be it makes to change it to {{ quote .hintMessage }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I already did this and added the support for markdown on the UI side as well

@@ -121,6 +121,17 @@ export default {
}
return join(hintText, ' / ')
},
hintColor () {
if (this.machineImage.expirationDate ||
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we create a class for this classification, version, hint, ... stuff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, would be a good idea. But not as part of this PR

@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 23, 2021
@grolu grolu requested a review from petersutter July 23, 2021 15:12
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 23, 2021
Co-authored-by: Holger Koser <holger.koser@sap.com>
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 23, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 23, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 23, 2021
@holgerkoser holgerkoser self-requested a review July 27, 2021 08:47
@grolu grolu dismissed petersutter’s stale review July 27, 2021 08:48

changes made

@gardener-robot gardener-robot added needs/review Needs review and removed reviewed/lgtm Has approval for merging labels Jul 27, 2021
@@ -36,6 +36,9 @@ SPDX-License-Identifier: Apache-2.0
{{item.name}} [{{item.version}}]
</span>
</template>
<template v-slot:message="{ message }">
<div v-html="message"></div>
Copy link
Member

@holgerkoser holgerkoser Jul 27, 2021

Choose a reason for hiding this comment

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

I think something like this would be better

 <template v-slot:message="{ message }">
   <multi-message :message="message"/>
 </template>

Copy link
Member

@holgerkoser holgerkoser Jul 28, 2021

Choose a reason for hiding this comment

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

CodeSandbox example:
https://codesandbox.io/s/focused-cdn-qn1wl?file=/src/components/MultiMessage.vue

<template>
  <div class="wrapper">
    <template v-for="({ type, hint }, index) in hints">
      <div v-if="type === 'html'" v-html="hint" :key="index" />
      <div v-else v-text="hint" :key="index" />
    </template>
  </div>
</template>

<script>
export default {
  props: {
    message: {
      type: String,
    },
  },
  computed: {
    hints() {
      try {
        const obj = JSON.parse(this.message);
        return Array.isArray(obj) ? obj : [obj];
      } catch (err) {
        return [
          {
            type: "text",
            hint: this.message,
          },
        ];
      }
    },
  },
};
</script>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -119,7 +122,18 @@ export default {
if (this.machineImage.isPreview) {
hintText.push('Preview versions have not yet undergone thorough testing. There is a higher probability of undiscovered issues and are therefore not recommended for production usage')
}
return join(hintText, ' / ')
return join(hintText, '<br />')
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid hand-craftet html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed as suggested

if (this.machineImage.needsLicense) {
hintText.push('The OS image selected requires a license and a contract for full enterprise support. By continuing you are confirming that you have a valid license and you have signed an enterprise support contract.')
if (this.machineImage.vendorHint) {
hintText.push(transformHtml(this.machineImage.vendorHint.hintMessage))
Copy link
Member

@holgerkoser holgerkoser Jul 27, 2021

Choose a reason for hiding this comment

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

hints.push({ 
  type: 'html', 
  hint: transformHtml(this.machineImage.vendorHint.hintMessage) 
})

or

hints.push({ 
  type: 'text', 
  hint: `Image version expires on: ${this.machineImage.expirationDateString}. Image update will be enforced after that date.`
})

and

return JSON.stringify(hints)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -465,7 +464,7 @@ const getters = {
expirationDate,
vendorName,
icon: vendorName,
needsLicense
vendorHint: findVendorHint(state.cfg.vendorHints, vendorName)
Copy link
Member

Choose a reason for hiding this comment

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

The vendorHint only depends on the vendor name and should not be determined outside of the iteratee.

        const vendorName = vendorNameFromImageName(machineImage.name)
        const vendorHint = findVendorHint(state.cfg.vendorHints, vendorName)

        return map(versions, ({ version, expirationDate, cri, classification }) => {
          return decorateClassificationObject({
            key: name + '/' + version,
            name,
            version,
            cri,
            classification,
            expirationDate,
            vendorName,
            icon: vendorName,
            vendorHint
          })
        })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 306 to 315
{{- range .Values.frontendConfig.vendorHints }}
- vendorNames:
{{- range .vendorNames }}
- {{ . }}
{{- end }}
hintMessage: {{ quote .hintMessage }}
{{- if .hintType }}
hintType: {{ .hintType }}
{{- end }}
{{- end }}
Copy link
Member

@holgerkoser holgerkoser Jul 28, 2021

Choose a reason for hiding this comment

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

The property is already called vendorHints. It would not repeat vendor and hint in child properties. Type from my point of view should be named severity. But this is only my personal opinion.

Suggested change
{{- range .Values.frontendConfig.vendorHints }}
- vendorNames:
{{- range .vendorNames }}
- {{ . }}
{{- end }}
hintMessage: {{ quote .hintMessage }}
{{- if .hintType }}
hintType: {{ .hintType }}
{{- end }}
{{- end }}
{{- range .Values.frontendConfig.vendorHints }}
- matchNames: {{ toJson .matchNames }}
message: {{ quote .message }}
{{- if .severity }}
severity: {{ .severity }}
{{- end }}
{{- end }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed as suggested

@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 30, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 30, 2021
Co-authored-by: Holger Koser <holger.koser@sap.com>
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 30, 2021
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 30, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 30, 2021
Copy link
Member

@holgerkoser holgerkoser left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Jul 30, 2021
@grolu grolu merged commit 53f90eb into master Jul 30, 2021
@petersutter petersutter deleted the enh/configurable_machine_image_warnings branch August 6, 2021 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging size/m Size of pull request is medium (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants