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

Trigger sanitizers workflow if label is added #37

Merged
merged 4 commits into from
Mar 17, 2021

Conversation

scpeters
Copy link
Contributor

@scpeters scpeters commented Mar 17, 2021

Check for the do-clang-test label in order to trigger the sanitizer workflow.

Part of maliput/maliput_infrastructure#177

Check for the do-clang-test label in order to trigger
the sanitizer workflow.
@scpeters
Copy link
Contributor Author

ok, it hasn't triggered the sanitizer tests just by opening the pull request; I'll now apply the label...

@scpeters scpeters added the do-clang-test Triggers clang builds in a pull request label Mar 17, 2021
@scpeters
Copy link
Contributor Author

ok, it just triggered a bunch of new builds

@scpeters
Copy link
Contributor Author

after they finish, I'll try adding an additional commit to see if the jobs keep triggering

@scpeters
Copy link
Contributor Author

looks like the tsan build failed

@agalbachicar
Copy link
Collaborator

looks like the tsan build failed

Having tsan for maliput_dragway is a bug. It is not prepared for it and we don't have any concurrent code to evaluate. Would you please remove it?

BTW the feature looks amazing!

agalbachicar
agalbachicar previously approved these changes Mar 17, 2021
Copy link
Collaborator

@agalbachicar agalbachicar left a comment

Choose a reason for hiding this comment

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

LGTM

@scpeters
Copy link
Contributor Author

looks like the tsan build failed

Having tsan for maliput_dragway is a bug. It is not prepared for it and we don't have any concurrent code to evaluate. Would you please remove it?

disabled tsan in e8157f6

I also disabled the builds for pushes to master branch

BTW the feature looks amazing!

:)

@scpeters
Copy link
Contributor Author

no clang builds were triggered for my most recent commit, which is what I expected, since I expect it to only trigger a build when a label is added

I'm going to try adding a different label. I expect that it will trigger a build, since the logic currently checks for the presence of the do-clang-test label when a new label is added, but I don't think it's able to identify if the label was just added or if it was pre-existing

@scpeters scpeters added the enhancement New feature or request label Mar 17, 2021
@scpeters
Copy link
Contributor Author

adding the enhancement label triggered clang builds as I expected.

it looks like I didn't fully disable tsan though

@scpeters scpeters added do-clang-test Triggers clang builds in a pull request and removed enhancement New feature or request do-clang-test Triggers clang builds in a pull request labels Mar 17, 2021
@agalbachicar
Copy link
Collaborator

I'm going to try adding a different label. I expect that it will trigger a build, since the logic currently checks for the presence of the do-clang-test label when a new label is added, but I don't think it's able to identify if the label was just added or if it was pre-existing

What if we just remove and add again the same label?

@scpeters
Copy link
Contributor Author

I'm going to try adding a different label. I expect that it will trigger a build, since the logic currently checks for the presence of the do-clang-test label when a new label is added, but I don't think it's able to identify if the label was just added or if it was pre-existing

What if we just remove and add again the same label?

I just tested that as well and that works. The point of this test was to confirm that the logic is currently not checking the content of the new label, just the content of all current labels. I think it would be a little nicer to check the newly added label, but I'm not sure exactly how to do that. I think it's pretty good like this though.

Let me know if that didn't make sense

@agalbachicar
Copy link
Collaborator

Let's move forward with this as is. I can help to deploy this in all repositories.

Copy link
Collaborator

@agalbachicar agalbachicar left a comment

Choose a reason for hiding this comment

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

LGTM

@scpeters
Copy link
Contributor Author

Let's move forward with this as is. I can help to deploy this in all repositories.

thanks!

@scpeters scpeters merged commit 78bf499 into master Mar 17, 2021
@scpeters scpeters deleted the scpeters/sanitizers_workflow_on_label branch March 17, 2021 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-clang-test Triggers clang builds in a pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants