Check all places with check_mode: no for side effects

[fungusakafungus]

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Fixes the last place where running with --check might change files. Noticed by @tjanson in #8570

Also removes notify from this task as the task has changed_when: false
and notify is not going to fire.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Remove `check_mode: no` from gen_certs_script.yml to prevent changing files

[k8s-ci-robot]

Hi @fungusakafungus. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[tjanson]

I haven't checked what the notify triggers (and so far never did, due to changed_when: false). Should it really be removed or is that another bug?

[fungusakafungus]

or is that another bug?

yes 🙈

[fungusakafungus]

I checked other files for changed_when: false + notify: ... bug, this is the only place.

[cristicalin]

It never occurred to me that kubespray could do a full run with --check. Does this work pre-deployment or only on deployed environments? Maybe we should add this to the CI to ensure we don't break it in the future.

[tjanson]

It never occurred to me that kubespray could do a full run with --check. Does this work pre-deployment or only on deployed environments?

Unfortunately it does not work as there are steps such as creating temporary directories for downloads which the succeeding tasks depend on. (This should affect both deployed and "empty" envs.) At brief glance it seems those problems may be fixable at least for deployed envs.

What should certainly not happen is that check mode causes destructive actions. I personally think having check mode work on deployed envs (so that it can be used for its intented purpose of checking what would happen) would be extremely beneficial for users.

There was a previous issue about this, #6870, but that went stale.

[floryut]

@fungusakafungus Thank you, hope that the last of it ;)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floryut, fungusakafungus

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:

• OWNERS [floryut]

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

[cristicalin]

Thanks for the explanation @tjanson !
We could add a CI test for running with --check at the end of a deployment and allow it initially to fail until we iron out the remaining bugs. It makes sense to see that the code does not break this functionality.

/lgtm

[fungusakafungus]

@floryut I think the release note is misleading, this did not remove check_mode: no, it just removed the last one that had side effects. There are still many check_mode: no tasks that run commands to gather information. So to me, this PR shouldn't result in a release note.

[floryut]

oh, my bad, I've updated as I still think this should be included in the release note. Do you think we should remove it entirely ?

[fungusakafungus]

There are other instances of check_mode: no in gen_certs_script.yml, but I guess the release note is ok as it is now.