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

[SDK Suppressions labels] Add new action to update suppressions labels #31878

Open
wants to merge 189 commits into
base: main
Choose a base branch
from

Conversation

JackTn
Copy link
Member

@JackTn JackTn commented Dec 13, 2024

add new action to update sdk-suppression label

Overview :
Add sdk-suppressions file to process sdk breaking change

Changes:

  • [.github/workflows/SDK-Suppressions-Label.yaml] main action added trigger by pull request
  • [.github/workflows/update-labels.yaml] add trigger to update label
  • [eng/tools/package.json] dependence config for sdk-suppressions
  • [eng/tools/sdk-suppressions] main code to process sdk-suppressions label base diff by pr sdk-suppression.yaml file

Copy link

openapi-pipeline-app bot commented Dec 13, 2024

Next Steps to Merge

Next steps that must be taken to merge this PR:
  • ❌ The required check named SpellCheck has failed. Refer to the check in the PR's 'Checks' tab for details on how to fix it and consult the aka.ms/ci-fix guide
  • ❌ The required check named Protected Files has failed. Refer to the check in the PR's 'Checks' tab for details on how to fix it and consult the aka.ms/ci-fix guide

Copy link

openapi-pipeline-app bot commented Dec 13, 2024

PR validation pipeline restarted successfully. If there is ApiView generated, it will be updated in this comment.

@JackTn JackTn force-pushed the jacktn/suppression-action branch from 1401313 to ec018c2 Compare December 17, 2024 11:47
@JackTn JackTn marked this pull request as ready for review December 17, 2024 13:05
Copy link
Member

@mikeharder mikeharder left a comment

Choose a reason for hiding this comment

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

Address review feedback

@JackTn JackTn force-pushed the jacktn/suppression-action branch from 6cb5e49 to 5b11a33 Compare December 19, 2024 14:28
@JackTn JackTn requested review from mikeharder and raych1 December 19, 2024 14:30
@raych1
Copy link
Member

raych1 commented Dec 19, 2024

@mikeharder could you review again?

@JackTn JackTn force-pushed the jacktn/suppression-action branch from 62f6013 to be7b892 Compare December 20, 2024 05:32
@JackTn JackTn requested a review from raych1 December 20, 2024 06:45
@microsoft-github-policy-service microsoft-github-policy-service bot added the no-recent-activity There has been no recent activity on this issue. label Jan 6, 2025

- uses: ./.github/actions/add-label-artifact
name: Upload artifact with results-go
if: ${{ steps.run-suppressions-script.outputs.BreakingChange-Go-Sdk-Suppression }}
Copy link
Member

Choose a reason for hiding this comment

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

I think you want to set the label in either case, both true and false correct? Both are needed to ensure labels are removed when they no longer apply.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, true to add and false to remove

Copy link
Member

Choose a reason for hiding this comment

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

But doesn't the if condition on line 76 mean you only call add-label-artifact when the condition is true?

Copy link
Member Author

@JackTn JackTn Jan 7, 2025

Choose a reason for hiding this comment

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

I understand the reason for your confusion. For instance, on line 75, the step output value is 'true' or 'false', which is not a proper boolean. It's a string. Therefore, writing if 'false' will still execute. And this is my test case JackTn#17 , can you see it?

Copy link
Member

Choose a reason for hiding this comment

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

Why do you need the if condition at all? Do you need a tri-state value ("true", "false", and undefined), where undefined means neither add nor remove the label)? If so, I recommend changing the code to be more clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

The status value should have only two cases: value exists (either true or false) or undefined. If the value exists, it indicates execution, where labels are added or removed based on true or false. If it is undefined, no operation is performed.

Copy link
Member Author

Choose a reason for hiding this comment

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

The purpose of using an if condition is to determine whether this step needs to be executed.

Copy link
Member

Choose a reason for hiding this comment

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

@JackTn , can you add comment inline to explain this behavior for better readability?

Copy link
Member Author

Choose a reason for hiding this comment

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

added

Copy link
Member

@mikeharder mikeharder left a comment

Choose a reason for hiding this comment

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

Change dependency, and verify if label artifacts should be generate for both true and false.

@microsoft-github-policy-service microsoft-github-policy-service bot removed the no-recent-activity There has been no recent activity on this issue. label Jan 7, 2025
@JackTn JackTn requested a review from mikeharder January 7, 2025 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants