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

fix: Remove previous RELATED_IMAGES before adding new ones #967

Merged
merged 1 commit into from
Jul 26, 2021
Merged

Conversation

tolusha
Copy link
Contributor

@tolusha tolusha commented Jul 26, 2021

Signed-off-by: Anatolii Bazko [email protected]

What does this PR do?

Remove previous RELATED_IMAGES before adding new ones to config/manager/manager.yaml

Screenshot/screencast of this PR

N/A

What issues does this PR fix or reference?

eclipse-che/che#20171

How to test this PR?

Run twice, make sure that content of the config/manager/manager.yaml is not changes after the second try.
./olm/addDigests.sh -w . -s bundle/stable/eclipse-che-preview-openshift/manifests/che-operator.clusterserviceversion.yaml -o config/manager/manager.yaml -t 7.32.0

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

envVarName=$(cat "${OPERATOR_FILE}" | yq -r '.spec.template.spec.containers[0].env['${i}'].name')
if [[ ${envVarName} =~ plugin_registry_image ]] || [[ ${envVarName} =~ devfile_registry_image ]]; then
yq -riY 'del(.spec.template.spec.containers[0].env['${i}'])' ${OPERATOR_FILE}
i=$((i-1))
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I'd remove this decrement and put the following increment into else block.
  2. Shouldn't you change the total number of env vars too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I've thought about it but this approach sounds more explicit
  2. It does not matter, there are no errors, yq returns null as a string

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I have opposite opinion, but ok.
  2. That's logically wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. It is not a c++ or java. We have to processed all records, other doesn't matter

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a source of errors. If yq behavior changes or someone add another line that doesn't tolerate index error... but up to you...

olm/addDigests.sh Show resolved Hide resolved
@openshift-ci
Copy link

openshift-ci bot commented Jul 26, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: flacatus, mmorhun, tolusha

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mmorhun mmorhun assigned tolusha and unassigned mmorhun and flacatus Jul 26, 2021
@tolusha tolusha merged commit 833b3aa into main Jul 26, 2021
@tolusha tolusha deleted the 20171 branch July 26, 2021 09:38
@che-bot che-bot added this to the 7.34 milestone Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants