spiffe / helm-charts-hardened

Apache License 2.0
17 stars 31 forks source link

Change cleanup default #349

Closed kfox1111 closed 4 months ago

kfox1111 commented 5 months ago

Ready to undraft as soon as we're ready to cut a release.

edwbuck commented 4 months ago

Looks good, minor ask on the release notes.

kfox1111 commented 4 months ago

Please cut out the one line that roughly says "if you're doing it right, then you don't need to do anything" and clarify by making the follow up paragraph say, "if you have skipped releases in the past" you need to do X, Y, or Z.

In general, can you please propose changes as a github suggestion so that:

  1. you get marked as contributing and so you get credit for it.
  2. that the suggestion has all the details filled out as you would like them, so there are not misunderstandings that cause another review pass

For this particular suggestion, I was trying to make it clear that, if you follow directions you dont need to care about any of the rest of the note. If you care about details, read on, and if you dont follow directions, here's how you fix it when it breaks. Was really trying to avoid burdening those that follow directions with details they probably don't need, and not make it sound like we condone following unsupported upgrade procedures.

If this is too much info, we could just drop the upgrade note, in its entirely. If they follow the procedure, no action is required. If they don't follow directions, they can ask for help?

faisal-memon commented 4 months ago

This looks good to me. @edwbuck did you have any further suggestions?

edwbuck commented 4 months ago

Please cut out the one line that roughly says "if you're doing it right, then you don't need to do anything" and clarify by making the follow up paragraph say, "if you have skipped releases in the past" you need to do X, Y, or Z.

In general, can you please propose changes as a github suggestion so that:

1. you get marked as contributing and so you get credit for it.

2. that the suggestion has all the details filled out as you would like them, so there are not misunderstandings that cause another review pass

For this particular suggestion, I was trying to make it clear that, if you follow directions you dont need to care about any of the rest of the note. If you care about details, read on, and if you dont follow directions, here's how you fix it when it breaks. Was really trying to avoid burdening those that follow directions with details they probably don't need, and not make it sound like we condone following unsupported upgrade procedures.

If this is too much info, we could just drop the upgrade note, in its entirely. If they follow the procedure, no action is required. If they don't follow directions, they can ask for help?

Kevin,

  1. I don't care about getting credit for the change.
  2. The suggestion to cut the line is complete, in the sense that the whole line can be removed.

I don't believe we need to provide instructions for people who aren't following supported procedures, because unsupported procedures are unsupported. But, if you want to provide such procedures, the two paragraphs in the release notes should stand as one paragraph. Standing alone, reading

"If you are following procedures correctly, you don't need to do anything"

is just confusing. The reader hasn't been given any context to know what violation of procedure they might have performed that would tell them they need to act upon, and when they fear they might have incorrectly done something, they aren't directed to what they need to do.

Simply cutting the statement is desirable. As I mentioned before, if you feel this statement is more about the second item in the release notes, then combine it into the second item.

kfox1111 commented 4 months ago

In general, can you please propose changes as a github suggestion so that:

1. you get marked as contributing and so you get credit for it.

2. that the suggestion has all the details filled out as you would like them, so there are not misunderstandings that cause another review pass

For this particular suggestion, I was trying to make it clear that, if you follow directions you dont need to care about any of the rest of the note. If you care about details, read on, and if you dont follow directions, here's how you fix it when it breaks. Was really trying to avoid burdening those that follow directions with details they probably don't need, and not make it sound like we condone following unsupported upgrade procedures. If this is too much info, we could just drop the upgrade note, in its entirely. If they follow the procedure, no action is required. If they don't follow directions, they can ask for help?

Kevin,

  1. I don't care about getting credit for the change.
  2. The suggestion to cut the line is complete, in the sense that the whole line can be removed.

Hence the "in general" prefix on the statement. Not necessary for this particular review item, but for suggestions from you, in general.

I don't believe we need to provide instructions for people who aren't following supported procedures, because unsupported procedures are unsupported.

I'd agree if it wasn't for needing to help people recently on slack fix setups they broke when they skipped upgrades. :/

"If you are following procedures correctly, you don't need to do anything"

But, if you want to provide such procedures, the two paragraphs in the release notes should stand as one paragraph. Standing alone, reading is just confusing. The reader hasn't been given any context to know what violation of procedure they might have performed that would tell them they need to act upon, and when they fear they might have incorrectly done something, they aren't directed to what they need to do.

Simply cutting the statement is desirable. As I mentioned before, if you feel this statement is more about the second item in the release notes, then combine it into the second item.

This has already been done, and was when you wrote this response. Please review the current pr, not an older version.

What, if anything is left to get this merged so we can finally cut a release? I think all concerns have been addressed.

edwbuck commented 4 months ago

This has already been done, and was when you wrote this response. Please review the current pr, not an older version.

What, if anything is left to get this merged so we can finally cut a release? I think all concerns have been addressed.

Kevin, when a comment is made against a section of code in Github, github considers it a blocking item requiring resolution. It is displayed differently and will block the merging capabilites, even if such an item later gets buried by file changes.

If you reply to those items, instead of creating new non-attached top-level comments, it greatly eases the work of performing a review, and thus, the reviews move more quickly (and avoid these kinds of mistakes). I'll pay more attention to following this too, as I know that I've made such errors in the past, but if you see something like this in the future, please double-check that the issue raised against the code block gets an update.

Thanks. And I gave this a passing review. @faisal-memon can complete his review and merge it, when he gets the opportunity.