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

Jenkinsfile: qualityGate DELTA #990

Closed
wants to merge 1 commit into from
Closed

Jenkinsfile: qualityGate DELTA #990

wants to merge 1 commit into from

Conversation

jukzi
Copy link
Contributor

@jukzi jukzi commented Dec 7, 2023

do not fail build if total number of warnings have been reduced

see #989 (comment)

do not fail build if total number of warnings have been reduced
@jukzi jukzi requested a review from laeubi December 7, 2023 07:02
@laeubi
Copy link
Contributor

laeubi commented Dec 7, 2023

I think this is best reviewed by @akurtakov in general I think changes should not introduce new warnings (or use @suppresswarning if unavoidable) but if it helps we allow to introduce a new one if another is fixed instead I can't judge.

Copy link

github-actions bot commented Dec 7, 2023

Test Results

     270 files  ±0       270 suites  ±0   46m 46s ⏱️ + 3m 56s
  3 329 tests ±0    3 299 ✔️ ±0  30 💤 ±0  0 ±0 
10 284 runs  ±0  10 194 ✔️ ±0  90 💤 ±0  0 ±0 

Results for commit dd481a1. ± Comparison against base commit eeff809.

@akurtakov
Copy link
Member

I think I gave a better fix at the linked PR which removes the need for the "workaround" field. This happened only because of the threshold gate pointing the new issue thus I believe we should keep things as they are right now and not accept this one.

@jukzi
Copy link
Contributor Author

jukzi commented Dec 7, 2023

I think I gave a better fix at the linked PR which removes the need for the "workaround" field. This happened only because of the threshold gate pointing the new issue thus I believe we should keep things as they are right now and not accept this one.

I totally agree that that particular solution is better, however in general it would be good to just reduce warnings insted of complaining about moved warnings.

@akurtakov
Copy link
Member

Let's try with stricter rules, if/when we have it preventing to fix things I will change my mind immediately.
FWIW, I consider this one an experiment about how strict we can go in order to get cleaner codebase.

@akurtakov
Copy link
Member

Please don't close this PR for now - it may be something we just apply soon if needed.

@laeubi laeubi marked this pull request as draft December 7, 2023 07:48
@laeubi
Copy link
Contributor

laeubi commented Dec 7, 2023

Please don't close this PR for now - it may be something we just apply soon if needed.

I converted it to a draft in the meanwhile.

@laeubi
Copy link
Contributor

laeubi commented Dec 7, 2023

By the way, have you noticed how all checks are green now (including licence check)?

@jukzi
Copy link
Contributor Author

jukzi commented Dec 7, 2023

By the way, have you noticed how all checks are green now (including licence check)?

gj!

@akurtakov
Copy link
Member

Step by step, we are really going towards far better development process! Of course it has its growing pains but there is always a price.

@HannesWell
Copy link
Member

HannesWell commented Dec 10, 2023

By the way, have you noticed how all checks are green now (including licence check)?

That's very nice. 👍🏽

I think I gave a better fix at the linked PR which removes the need for the "workaround" field. This happened only because of the threshold gate pointing the new issue thus I believe we should keep things as they are right now and not accept this one.

I totally agree that that particular solution is better, however in general it would be good to just reduce warnings insted of complaining about moved warnings.

Of course the best would be to not introduce new warnings, but having one new if ten are fixed is also a win.
However will this setting also permit a one-to-one replacement? Then I would say it is usually not an advantage.

@laeubi
Copy link
Contributor

laeubi commented Dec 10, 2023

if one thinks the change is good even though it introduce a new warning there is always the option to merge it anyways for "edge-cases"...

@akurtakov
Copy link
Member

@HannesWell as PL please make a decision on this one and either merge or close. It's good to keep the PRs queue shorter :) .

@HannesWell
Copy link
Member

@HannesWell as PL please make a decision on this one and either merge or close. It's good to keep the PRs queue shorter :) .

I agree with Christoph's opinion on this. In general it should be attempted to not introduce new warnings and I'm convinced it is usually possible. But yes, in edge-cases it is still possible to overrule a failing quality-gate and submit changes nevertheless.

@HannesWell HannesWell closed this Jan 14, 2024
@HannesWell HannesWell deleted the jukzi-patch-1 branch January 14, 2024 16:48
@laeubi
Copy link
Contributor

laeubi commented Jan 17, 2024

I just wanted to make you aware that there is an open pull request to add an option to only fail the quality gate but do not change the overall build result here:

so if anyone is interested please express your support there!

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.

4 participants