channels/candidate-*: Drop YAML comments #379
Conversation
The comments made it harder for ART to auto-generate candidate
promotion pull requests, because they needed to either use a
comment-preserving YAML engine or blindly pattern match on strings
instead of using a YAML engine to inject new versions. We don't
really care about particular patch releases before they land in
candidate. If ART wants to explain why it skipped patch numbers, they
can do that in the pull request and/or commit message that adds the
next version (e.g. 4.1.34 commit/PR could explain why 4.1.32 and
4.1.33 were skipped).
Generated with:
$ for F in channels/candidate-4.*; do A="$(grep -v '^$\|^#' "${F}")"; echo "${A}" > "${F}"; done
openshift-ci-robot
added
the
approved
|
/lgtm Lets have ART and architects folks notified |
openshift-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vrutkovs, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
LalatenduMohanty
left a comment
LalatenduMohanty
left a comment
There was a problem hiding this comment.
-1 for removing the comments at this point of time. May be 6 months down the line. What is the benefit of removing the comments? Why are we removing comments? I have not heard ART complaining about it recently.
|
that's how we currently create cincinnati PRs: https://github.com/openshift/aos-cd-jobs/blob/master/pipeline-scripts/release.groovy readYaml claims to use SnakeYAML under the hood. That said, I don't personally remember any automated PR going wrong, removing comments ... but I'd recommend to invite @jupierce to this discussion. |
|
Yeah, this change will remove the need for gymnastics like: # We want to insert the previous minors right after versions: so they stay above other entries.
# Why not set it in right before the next minor begins? Because we don't confuse a comment line that might exist above the next minor.
# First, create a file with the content we want to insert
echo -n > ul.txt # Clear from previous channels
for urn in ${releasesForUpgradeChannel.join(' ')} ; do
echo "- \$urn" >> ul.txt # add the entry to lines to insert
done
echo >> ul.txt
rm -f slice* # Remove any files from previous csplit runs
csplit ${upgradeChannelFile} '/versions:/+1' --prefix slice # create slice00 (up to and including versions:) and slice01 (everything after)
cat slice00 ul.txt slice01 > ${upgradeChannelFile}ART could just add the version to the structured, ingested YAML and write structured YAML back to the file. |
|
@wking: PR needs rebase. DetailsInstructions 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. |
openshift-ci-robot
added
the
needs-rebase
|
pretty sure if we wrote structured yaml it would not include the comments. most yaml libs don't have a way to preserve them. they're certainly not functionally important, but if we'd like them to be there for information (certainly some entries could use an explanation), it would be better to incorporate them as an attribute in the data. it's true that the explanation could be in PR text, but that's not as easy to find. i don't know that anyone is clamoring to change this - once the "gymnastics" are written, they only become a concern when we need to change them. i don't really think there is a strong argument for or against keeping these comments. |
|
👍 If we find value in recording this data we should at least record it in a structured manner. |
|
I find no value in recording this data for the candidate channel. I do find value in recording tombstones on later promotions, but that's #75. |
|
@wking: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
#526 got rid of most of these, now they only exist in 4.2 and older channels |
|
@sdodson: Closed this PR. DetailsIn response to this:
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. |
7 participants
The comments made it harder for ART to auto-generate candidate promotion pull requests, because they needed to either use a comment-preserving YAML engine or blindly pattern match on strings instead of using a YAML engine to inject new versions. We don't really care about particular patch releases before they land in candidate. If ART wants to explain why it skipped patch numbers, they can do that in the pull request and/or commit message that adds the next version (e.g. 4.1.34 commit/PR could explain why 4.1.32 and 4.1.33 were skipped).
Generated with:
$ for F in channels/candidate-4.*; do A="$(grep -v '^$\|^#' "${F}")"; echo "${A}" > "${F}"; done