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

feat(STONEINTG-613): remove status report from PLR controller #359

Merged
merged 1 commit into from
Oct 23, 2023
Merged

feat(STONEINTG-613): remove status report from PLR controller #359

merged 1 commit into from
Oct 23, 2023

Conversation

hongweiliu17
Copy link
Contributor

@hongweiliu17 hongweiliu17 commented Oct 11, 2023 •
edited
Loading

  • remove status report from integrationPLR controller
  • create comment to PR in statusreport controller for github webhook
  • add test result to report of statusreport
  • update comment if there is one existing comment of snapshot and scenario test status
  • update controller diagram

checkRun example: https://github.com/hongliuorg/devfile-sample-go-basic/pull/4/checks?check_run_id=16776195643
commitStatus/comment example: https://github.com/hongweiliu17/devfile-sample-go-basic/pull/2

Maintainers will complete the following section

  • Commit messages are descriptive enough (hints)
  • Code coverage from testing does not decrease and new code is covered
  • Controllers diagrams are updated when PR changes controllers code (if applicable)

@hongweiliu17 hongweiliu17 marked this pull request as draft October 12, 2023 05:23
@hongweiliu17 hongweiliu17 marked this pull request as ready for review October 12, 2023 05:44
@hongweiliu17 hongweiliu17 force-pushed the STONEINTG-613 branch 5 times, most recently from 85ac32d to 1d2d95e Compare October 17, 2023 08:19
@hongweiliu17 hongweiliu17 force-pushed the STONEINTG-613 branch 2 times, most recently from e9b87e6 to cfe5395 Compare October 17, 2023 09:56
dirgim
dirgim reviewed Oct 17, 2023
View reviewed changes
status/reporters.go Outdated Show resolved Hide resolved
dirgim
dirgim approved these changes Oct 17, 2023
View reviewed changes
Copy link
Collaborator

@dirgim dirgim left a comment

Choose a reason for hiding this comment

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

LGTM aside the tiny nit!

@openshift-ci openshift-ci bot added the lgtm label Oct 17, 2023
@openshift-ci openshift-ci bot removed the lgtm label Oct 18, 2023
dheerajodha
dheerajodha reviewed Oct 18, 2023
View reviewed changes
Copy link
Member

@dheerajodha dheerajodha left a comment

Choose a reason for hiding this comment

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

small comment about function names

controllers/statusreport/statusreport_adapter.go Outdated Show resolved Hide resolved
controllers/integrationpipeline/integrationpipeline_controller.go Show resolved Hide resolved
@hongweiliu17
feat(STONEINTG-613): remove status report from PLR controller
f687659 
* remove status report from integrationPLR controller
* create comment in statusreport controller for github webhook
* add test result to report of statusreport
* update comment when one comment exists for the snapshot and scenario
* update controller diagram

Signed-off-by: Hongwei Liu <hongliu@redhat.com>
MartinBasti
MartinBasti approved these changes Oct 18, 2023
View reviewed changes
Copy link
Collaborator

@MartinBasti MartinBasti left a comment

Choose a reason for hiding this comment

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

Looks good!

Given the size of PR, please do careful testing before merging.

@openshift-ci openshift-ci bot added the lgtm label Oct 18, 2023
@hongweiliu17
Copy link
Contributor Author

Tested it again in my personal cluster, it works, no error. Will merge it next Monday. Thanks all for the review!

@hongweiliu17 hongweiliu17 merged commit e85e2e6 into konflux-ci:main Oct 23, 2023
11 checks passed
@rh-tap-build-team rh-tap-build-team bot mentioned this pull request Oct 23, 2023
Merged
@hongweiliu17 hongweiliu17 deleted the STONEINTG-613 branch December 14, 2023 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dheerajodha dheerajodha dheerajodha left review comments

@dirgim dirgim dirgim approved these changes

@MartinBasti MartinBasti MartinBasti approved these changes

@14rcole 14rcole Awaiting requested review from 14rcole 14rcole is a code owner

@Josh-Everett Josh-Everett Awaiting requested review from Josh-Everett Josh-Everett is a code owner

@jsztuka jsztuka Awaiting requested review from jsztuka jsztuka is a code owner

@jencull jencull Awaiting requested review from jencull jencull is a code owner

Assignees

@MartinBasti MartinBasti

@dirgim dirgim

Labels
lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hongweiliu17 @MartinBasti @dirgim @dheerajodha @openshift-merge-robot