Ignore comments in config-ordering checks

[petr-muller]

...and put some old comments back

Note that the automated tools will still remove them (because unfortunately Go YAML libraries do not seem to have the capability to preserve comments on round-trip) but we can still add them and spot their removal during PRs.

[petr-muller]

/cc @stevekuznetsov @droslean @bbguimaraes

[wking]

I haven't tracked the others all down, but it looks like there are more we may want to restore:

$ git show 0f21ffa0 | grep '^[-+] *#'
-      # this job does not create a release artifact
-      # artifacts: images
+          #!/bin/bash
-          #!/bin/bash
+          #!/bin/bash
-          #!/bin/bash
+          #!/bin/bash
-          #!/bin/bash
-    # The abomination below is equivilent to `^((?!Documentation).)*$`. Since
-    # Go doesn't support negative lookaheads, we are stuck with the following.
+          #!/bin/bash
-          #!/bin/bash
-          #!/bin/bash
+          #!/bin/bash
-          #!/bin/bash
+          #!/bin/bash
-          #!/bin/bash
+          #!/bin/bash
-          #!/bin/bash
+          #!/bin/bash
-          #!/bin/bash
+          #!/bin/bash
-          #!/bin/bash
-        #no targets, run everything
+          #!/bin/bash
-          #!/bin/bash
+          #!/bin/bash
-          #!/bin/bash
-    # TODO: Remove once the job is stable
-    # TODO: Remove once the job is stable
-          # TODO: the team has promised me they will be able to remove this soon
-          # TODO: the team has promised me they will be able to remove this soon
$ git show 0f21ffa0 | grep '^[-+] *#!/bin' | sort | uniq -c
     11 +          #!/bin/bash
     12 -          #!/bin/bash

[wking]

Ah, the extra #!/bin/bash removal was just this being converted to the single line (!) here. I dunno if it's worth unwinding that too. I personally find the multi-line form more readable, but I'm not sure how easy it would be to have rewrite tooling preserve the original forms.

To track the other changes to files, you can use:

$  git show 0f21ffa0 | grep '^[-+] *#\|diff ' | grep -v /bin/bash | grep -B1 '^[-+] *#'
diff --git a/ci-operator/jobs/openshift/ci-operator/openshift-ci-operator-postsubmits.yaml b/ci-o
erator/jobs/openshift/ci-operator/openshift-ci-operator-postsubmits.yaml
-      # this job does not create a release artifact
-      # artifacts: images
--
diff --git a/ci-operator/jobs/openshift/installer/openshift-installer-presubmits.yaml b/ci-operat
r/jobs/openshift/installer/openshift-installer-presubmits.yaml
-    # The abomination below is equivilent to `^((?!Documentation).)*$`. Since
-    # Go doesn't support negative lookaheads, we are stuck with the following.
--
diff --git a/ci-operator/jobs/openshift/openshift-azure/openshift-openshift-azure-presubmits.yaml
b/ci-operator/jobs/openshift/openshift-azure/openshift-openshift-azure-presubmits.yaml
-        #no targets, run everything
--
diff --git a/ci-operator/jobs/openshift/origin/openshift-origin-presubmits.yaml b/ci-operator/job
/openshift/origin/openshift-origin-presubmits.yaml
-    # TODO: Remove once the job is stable
-    # TODO: Remove once the job is stable
diff --git a/ci-operator/jobs/openshift/os/openshift-os-presubmits.yaml b/ci-operator/jobs/opensh
ft/os/openshift-os-presubmits.yaml
-          # TODO: the team has promised me they will be able to remove this soon
-          # TODO: the team has promised me they will be able to remove this soon

[petr-muller]

Yeah, I knew there were more comments but I only wanted to invest the effort when @stevekuznetsov says OK to this one :)

Thanks for pointing out the bash collapse, though. That's not good, and I have not realized this would happen.

[stevekuznetsov]

We need to determine in which cases the bash gets inlined, because the generator seems to have done it only in some cases.

[petr-muller]

Let's keep this PR about the comments - I'll file an issue here for myself to investigate and fix the collapsed bash.

[stevekuznetsov]

Also as far as the comments go, I would say
/shrug

Not super bummed we are missing a regex complaint and some TODOs

[wking]

We need to determine in which cases the bash gets inlined...

I'd guess it depends on string length.

[wking]

Not super bummed we are missing a regex complaint and some TODOs

Maybe convert the TODOs into GitHub issues? I think we want a note for the regex though, otherwise folks confused by it have to drill through blame for an explaination.

[stevekuznetsov]

I think we want a note for the regex though, otherwise folks confused by it have to drill through blame for an explaination.

Gets a huge /shrug from me -- I'm almost thinking about just removing the line. The tests aren't that expensive.

[petr-muller]

shrug is fine, but I cannot merge without an /approve :)

It's not just about existing comments but also about ci/prow/ordered-prow-config not failing on them

[petr-muller]

about the collapsed bash: #1325

[stevekuznetsov]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: petr-muller, stevekuznetsov

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [stevekuznetsov]

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

[openshift-ci-robot]

@petr-muller: Updated the job-config configmap using the following files:

• key openshift-installer-presubmits.yaml using file ci-operator/jobs/openshift/installer/openshift-installer-presubmits.yaml

Details

In response to this:

...and put some old comments back

Note that the automated tools will still remove them (because unfortunately Go YAML libraries do not seem to have the capability to preserve comments on round-trip) but we can still add them and spot their removal during PRs.

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.