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

Add github dispatch for operator sync workflow #996

Merged

Conversation

maroroman
Copy link
Contributor

Prerequisite for: opendatahub-io/opendatahub-operator#205

Description

This workflow sends a dispatch whenever a pull request is merged that modifies the crd folder of the dashboard repository. The operator has a workflow that then copies these changes automatically.

How Has This Been Tested?

This PR was tested using https://github.com/nektos/act to locally verify that the workflow is sending the dispatch correctly.

To test this in real conditions, a fork of the operator and dashboard is required:

  1. Create a pull request in the odh-dashboard fork that changes a file in /manifests/crd folder
  2. Merge the PR into the main branch of the fork
  3. Verify that the action has been triggered
  4. Verify dispatch reached opendatahub-operator fork sync workflow and has triggered it.

Test Impact

No tests are done for github workflows (afaik)

Request review criteria:

  • The commits have meaningful messages (squashes happen on merge by the bot).
  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work
  • The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

@openshift-ci openshift-ci bot requested review from DaoDaoNoCode and lucferbux March 9, 2023 13:14
@maroroman maroroman added infrastructure Anything non feature/* related that improves general working of the Dashboard feature/notebook-controller KubeFlow NoteBook Controller (KFNBC) Feature labels Mar 9, 2023
@maroroman
Copy link
Contributor Author

Just a sidenote, I am still trying to make this work with my forks, but locally the command is fine, so it should work in real conditions as long as the trigger is correct.

@andrewballantyne andrewballantyne added do-not-merge/hold This PR is hold for some reason and removed infrastructure Anything non feature/* related that improves general working of the Dashboard feature/notebook-controller KubeFlow NoteBook Controller (KFNBC) Feature labels Mar 9, 2023
@andrewballantyne
Copy link
Member

How do we test this @maroroman ? @VaishnaviHire / @LaVLaS

@maroroman
Copy link
Contributor Author

maroroman commented Mar 13, 2023

How do we test this @maroroman ? @VaishnaviHire / @LaVLaS

@andrewballantyne I verified that the dispatch works and is received by the operator (my fork of it) through the utility I mentioned in the test description.

To test the trigger it could be used as well or we can use some forks through the steps I wrote. I had trouble with those forks but that trigger is (as far as I know) exactly as the documentation states, so it should work :D

@lucferbux
Copy link
Contributor

Can we have more context, this is handle by the deployer downstream, are we moving away from that these files? Is this only targeted to upstream? Just to make sure we then disable it downstream.

@lucferbux
Copy link
Contributor

@maroroman ^

@maroroman
Copy link
Contributor Author

maroroman commented Mar 23, 2023

Can we have more context, this is handle by the deployer downstream, are we moving away from that these files? Is this only targeted to upstream? Just to make sure we then disable it downstream.

Based on a conversation with @VaishnaviHire this should only be targeted at upstream.

Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

/cc @LaVLaS @VaishnaviHire
@LaVLaS or @VaishnaviHire please approve this before we will merge it.

This is a github action and I don't mind the inclusion in the dashboard repo, but this is not for our dashboard effort so I need someone from the manifests team to approve this functionality.

@lucferbux
Copy link
Contributor

The checkout action works, I've tested it locally to other repo (didn't want to break anything in the operator repo) and it's working.

@LaVLaS @VaishnaviHire if you can validate and greenlight this we can review it and merge it.

@VaishnaviHire
Copy link
Member

The checkout action works, I've tested it locally to other repo (didn't want to break anything in the operator repo) and it's working.

@LaVLaS @VaishnaviHire if you can validate and greenlight this we can review it and merge it.

Sorry for the delay. Yes this looks good, we can merge it.

Copy link
Contributor

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 10, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, lucferbux

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@lucferbux
Copy link
Contributor

/assign @andrewballantyne

@openshift-merge-robot openshift-merge-robot merged commit a317f55 into opendatahub-io:main Apr 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants