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 a Helm chart for deploying host-check as a Job #11

Merged
merged 22 commits into from
May 30, 2024

Conversation

ebeaty-cisco
Copy link
Contributor

@ebeaty-cisco ebeaty-cisco commented May 15, 2024

Create a Helm chart for deploying host-check as a Job.

Testing: manual testing of mainline flows, UT written for each field in the resultant manifest.

Note to reviewer: charts/host-check/.helmignore is copied from the xrd-vrouter chart.

@ebeaty-cisco ebeaty-cisco marked this pull request as ready for review May 21, 2024 15:10
@@ -36,3 +36,7 @@ jobs:
- name: "Run vRouter unit tests"
run: |
docker run -v "$PWD/:/charts" helm-tests bats tests/ut/xrd-vrouter

- name: "Run host-check unit tests"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be merged with #10?

# There are required fields which must be specified for all installations:
# - image.repository
# - image.tag
# - platforms
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# - platforms
# - targetPlatforms

}

@test "host-check: Image tag must be specified" {
template_failure_no_set --set 'image.repository=local'\
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
template_failure_no_set --set 'image.repository=local'\
template_failure_no_set --set 'image.repository=local' \

}

@test "host-check Job: Name consists of the release name and chart name" {
template --set 'targetPlatforms[0]=xrd-vrouter'
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a shame that the targetPlatforms has to be set for each success test case.

I wonder if it would be better to move template into a per-chart ./utils.bash. This can use a template_no_set from ../utils.bash, which templates without setting, checks success, yamllint, kubeconform, etc.

Each per-chart MUT loads ./utils.bash instead.

The chart_dir functions can then be moved into the per-chart ./utils.bashs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implemented this suggestion

"additionalProperties": false
},
"targetPlatforms": {
"description": "List of platforms to run the host-check on",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"description": "List of platforms to run the host-check on",
"description": "List of platforms to run host-check on",

or "the host-check application"

"items": {
"type": "string",
"enum": [
"xrd-control-plane",
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the error message if you try and use an unsupported target platform?

We do some logic in the Helm template, rather than in the JSON Schema, so that error messages are sufficiently descriptive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with helm template logic with error message: targetPlatforms must be xrd-control-plane and/or xrd-vrouter, see test platforms must be xrd-vrouter or xrd-control-plane

Comment on lines 43 to 51
{{- define "hostCheck.args" -}}
{{- $arg := "" }}
{{- if and (has "xrd-control-plane" .Values.targetPlatforms) (has "xrd-vrouter" .Values.targetPlatforms) }}
{{- $arg = "" }}
{{- else }}
{{- $arg = printf "-p, %s" (.Values.targetPlatforms | first) }}
{{- end }}
{{- $arg }}
{{- end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{- define "hostCheck.args" -}}
{{- $arg := "" }}
{{- if and (has "xrd-control-plane" .Values.targetPlatforms) (has "xrd-vrouter" .Values.targetPlatforms) }}
{{- $arg = "" }}
{{- else }}
{{- $arg = printf "-p, %s" (.Values.targetPlatforms | first) }}
{{- end }}
{{- $arg }}
{{- end }}
{{- define "hostCheck.args" -}}
{{- $args := "" }}
{{- if and (has "xrd-control-plane" .Values.targetPlatforms) (has "xrd-vrouter" .Values.targetPlatforms) }}
{{- $args = "" }}
{{- else }}
{{- $args = printf "-p, %s" (.Values.targetPlatforms | first) }}
{{- end }}
{{- $args }}
{{- end }}


{{/*
Runtime arguments. If both xrd-control-plane and xrd-vrouter are specified,
the argument is not set.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
the argument is not set.
no arguments are set.

It may also be worth explicitly stating this is because the default behavior of host-check is to perform the checks for both platforms.

scripts/commit-check Outdated Show resolved Hide resolved
@ebeaty-cisco ebeaty-cisco merged commit 3035557 into main May 30, 2024
2 checks passed
@ebeaty-cisco ebeaty-cisco deleted the dev/add-host-check-app branch May 30, 2024 10:06
ebeaty-cisco added a commit that referenced this pull request Dec 19, 2024
Pull in commit-check script changes from main
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