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.

Sign up for GitHub

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

Jump to bottom

Matrix unique status #1840

Closed
wants to merge 8 commits into from
Closed

Matrix unique status #1840

wants to merge 8 commits into from

Conversation

6543
Copy link
Member

@6543 6543 commented Jun 8, 2023

Address #713

@6543 6543 added the server label Jun 8, 2023
@6543 6543 added this to the 1.0.0 milestone Jun 8, 2023
@woodpecker-bot
Copy link
Contributor

woodpecker-bot commented Jun 8, 2023 •
edited
Loading

Deployment of preview was successful: https://woodpecker-ci-woodpecker-pr-1840.surge.sh

@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2023 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (main@a4c791c). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1840   +/-   ##
=======================================
  Coverage        ?   39.04%           
=======================================
  Files           ?      179           
  Lines           ?    10805           
  Branches        ?        0           
=======================================
  Hits            ?     4219           
  Misses          ?     6289           
  Partials        ?      297

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@anbraten anbraten marked this pull request as draft June 9, 2023 10:42
@6543 6543 modified the milestones: 1.0.0, 1.1.0 Jun 11, 2023
@6543
Copy link
Member Author

6543 commented Jun 24, 2023

well the matrix summary workflow can be moved into it's own issue & pull

@6543 6543 modified the milestones: 1.1.0, 1.0.0 Jun 24, 2023
@6543 6543 marked this pull request as ready for review June 24, 2023 01:47
@6543 6543 changed the title WIP: Matrix unique status Matrix unique status Jun 24, 2023
@6543 6543 requested a review from a team June 24, 2023 01:49
qwerty287
qwerty287 approved these changes Jun 25, 2023
View reviewed changes
Copy link
Contributor

@qwerty287 qwerty287 left a comment

Choose a reason for hiding this comment

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

Didn't test. How does this look in the UI? The name shouldn't be shown there with the index

qwerty287
qwerty287 requested changes Jun 25, 2023
View reviewed changes
Copy link
Contributor

@qwerty287 qwerty287 left a comment

Choose a reason for hiding this comment

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

As of #1840 (review), please answer the question first

@6543
Copy link
Member Author

6543 commented Jun 25, 2023

Well i dont have a better idea beside the index, using matrix tags etc ... should it be first, second .... what about combinations ... etc

Beside the sanitation issues

@qwerty287
Copy link
Contributor

In general I'm fine with this, but the issue is that this is only needed for the forge status, but neither internally nor in the UI. This means that it should not be shown there.

You could solve this by using a separate column axis_index (or whatever) which is then used to generate the status context sent to the forge - but nowhere else as long as we don't need it.

@pat-s pat-s modified the milestones: 1.0.0, 1.1.0 Jun 27, 2023
@6543 6543 mentioned this pull request Jul 8, 2023
Merged
@6543
Copy link
Member Author

6543 commented Jul 8, 2023

refactoring extracted to #1950

@anbraten anbraten added the bug Something isn't working label Oct 14, 2023
@qwerty287 qwerty287 mentioned this pull request Nov 1, 2023
Merged
@6543 6543 closed this in #2695 Nov 1, 2023
6543 added a commit that referenced this pull request Nov 1, 2023
@qwerty287 @6543
Unique status for matrix (#2695)
abb2f28 
implement this fix but with an additional field on workflows to not
change the workflow name

closes #1840 
closes #713

---------

Co-authored-by: 6543 <6543@obermui.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qwerty287 qwerty287 qwerty287 requested changes

Assignees
No one assigned
Labels
bug Something isn't working server
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@6543 @woodpecker-bot @codecov-commenter @qwerty287 @anbraten @pat-s