-
Notifications
You must be signed in to change notification settings - Fork 34
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
chore: deploy configmaps as a part of pipeline #375
Conversation
/cc @dominikholler |
/retest |
f8e1986
to
53844de
Compare
/retest |
release/pipelines/windows-efi-installer/windows-efi-installer.yaml
Outdated
Show resolved
Hide resolved
release/pipelines/windows-efi-installer/windows-efi-installer.yaml
Outdated
Show resolved
Hide resolved
/lgtm |
release/pipelines/windows-efi-installer/windows-efi-installer.yaml
Outdated
Show resolved
Hide resolved
- name: delete-imported-configmaps | ||
params: | ||
- name: SCRIPT | ||
value: oc delete -f $(params.autounattendXMLConfigMapsURL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we cache the config map somewhere? If it changes during the pipeline run, then the delete might not work as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can use the customizeConfigMapName
or autounattendConfigMapName
params to delete the correct configmaps instead of URL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem will be, if the URL contains multiple configmaps, then it will delete only single CM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you return the created config maps as a result in import-unattend-configmaps
and use that result to delete them in delete-imported-configmaps
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, because the task does not have results defined in manifest https://github.com/tektoncd/catalog/blob/main/task/openshift-client/0.2/openshift-client.yaml#L14
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you perhaps cache the ConfigMaps file in a Workspace instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I am not mistaken, the workspaces requires record in PipelinerRun (see the note in https://tekton.dev/docs/pipelines/workspaces/#specifying-workspaces-in-pipelineruns). That means, we would break triggering of the pipeline from the UI. Users would have to build their own manifest for triggering the pipeline, which would go against the idea of this pipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunate. I'm just concerned this will be handed back to us as a possible bug in the future. If there's not other way, maybe we can at least document it in the description or somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair point, let me update it tomorrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
documented in 4bda75d
this change will remove the step, when user has to manually create the necessary ConfigMaps needed by VM. They are now deployed and removed by the Pipeline. Added new parameter acceptEula - By setting this parameter to true, user is accepting Eula specified in autounattend.xml file in ConfigMap. If param is set to false, pipeline will exit immediatelly Signed-off-by: Karel Simon <[email protected]>
during the Pipeline run The contents of autounattendXMLConfigMapsURL may change during the Pipeline run. This commit documents that if this happens, Pipeline may not be able to remove all deployed ConfigMaps. Signed-off-by: Karel Simon <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 0xFelix, ksimon1 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
1 similar comment
/retest |
What this PR does / why we need it:
chore: deploy configmaps as a part of pipeline
this change will remove the step, when user has to manually create the necessary ConfigMaps needed by VM. They are now deployed and removed by the Pipeline.
Added new parameter acceptEula - By setting this parameter, user is agreeing to the applicable Microsoft end user license agreement(s) for each deployment or installation for the Microsoft product(s). If param is set to false, or empty, pipeline will exit immediatelly
Release note: