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

[JENKINS-65462] Do NOT trigger jobs for changes that status != GerritChangeStatus.NEW #432

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

chenhuahuan
Copy link
Contributor

@chenhuahuan chenhuahuan commented Apr 25, 2021

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your master branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@chenhuahuan chenhuahuan changed the title FIX: Do NOT trigger jobs for those status!= GerritChangeStatus.NEW FIX: Do NOT trigger jobs for changes that status!= GerritChangeStatus.NEW Apr 25, 2021
@chenhuahuan
Copy link
Contributor Author

@rsandell rsandell changed the title FIX: Do NOT trigger jobs for changes that status!= GerritChangeStatus.NEW [JENKINS-65462] Do NOT trigger jobs for changes that status != GerritChangeStatus.NEW Apr 30, 2021
@rsandell rsandell added the bug label Apr 30, 2021
Copy link
Member

@rsandell rsandell left a comment

Choose a reason for hiding this comment

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

Why?
The ticket doesn't seem to explain what the problem is and what the code fixes.
I would also like to see a unit test that recreates the issue.

@chenhuahuan
Copy link
Contributor Author

chenhuahuan commented Jul 7, 2021

Why?
The ticket doesn't seem to explain what the problem is and what the code fixes.
I would also like to see a unit test that recreates the issue.


  1. Just updated the ticket with more detail.
  2. Unit test brings too much extra work for me because the modified function doesn't have a unit test itself.

Please take a look at the ticket description , you will understand the issue and why this Pull Request fixs it.

@chenhuahuan chenhuahuan requested a review from rsandell July 7, 2021 13:23
@chenhuahuan chenhuahuan closed this Jul 7, 2021
@chenhuahuan chenhuahuan reopened this Jul 7, 2021
@rsandell
Copy link
Member

I am getting a lot of connection timeouts in the existing unit tests with this "fix" applied.

@chenhuahuan
Copy link
Contributor Author

Doesn't make sense. What the timeout is about? It seems not related to the modification, is it some kind of CI environment issue?

@rsandell
Copy link
Member

The timeout is about the tests not running for too long time, and there are many of the unit/integration tests that fires a new event and waits for the build to start. So if the build doesn't start then it will wait until the test times out.

Which indicates that this change breaks a lot of assumptions about how the triggering is supposed to work.

@ckreisl
Copy link
Contributor

ckreisl commented Nov 11, 2022

The check if it's in state "NEW" might be at the wrong location. I think i got the idea why it should be in.
It's also related to the topic handling PR #468 in my name.

The first question is "When is a change interesting?"
For "Patchset Created" events the state should be always NEW.
In case the change gets abandoned during runtime an exception is raised that the plugin can't set a +/- 1 verified anymore after build completion.

But one might be interested in ref updated "MERGED" events.
In my case it gets interesting with enabled "Topic Association". From my current point of view changes assigned to a topic which are already "MERGED" or "ABANDONED" shouldn't be taken into account. That's why we should only check for the "NEW" state.

I already started to do some adaption but it's not finalized yet.

@rsandell
Copy link
Member

rsandell commented Dec 2, 2022

was this superseded by #481 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants