Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Change UT workflow to run against PRs #207

Conversation

ohltyler
Copy link
Contributor

@ohltyler ohltyler commented Jun 8, 2020

Issue #, if available: #206

Description of changes:

This PR changes the UT workflow by adding functionality to run against new pull requests made to the master branch

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ohltyler ohltyler force-pushed the changeUTWorkflow branch from 35f25e1 to 99b25ca Compare June 8, 2020 22:45
@ohltyler ohltyler linked an issue Jun 8, 2020 that may be closed by this pull request
repository: opendistro-for-elasticsearch/kibana-oss
ref: 7.7.0
token: ${{ secrets.KIBANA_OSS_ACCESS }}
repository: elastic/kibana
Copy link
Contributor

@ylwu-amzn ylwu-amzn Jun 8, 2020

Choose a reason for hiding this comment

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

Is it possible we pass elastic/kibana but fail opendistro-for-elasticsearch/kibana-oss ? As we are going to release plugin on ODFE, suggest to run test workflow on opendistro-for-elasticsearch/kibana-oss.

The root cause is we can't get secrets.KIBANA_OSS_ACCESS in forked branch? How about we talk with infra team to get this access key and configure it in our forked repo? Another way is we can discuss is it possible to make opendistro-for-elasticsearch/kibana-oss public.

Copy link
Contributor

Choose a reason for hiding this comment

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

The root cause is correct. However, if contributors outside of our team want to contribute to our product, they will be blocked by such secrets.

I think the reason we use elastic/kibana is that our product(AD Kibana plugin) is open-sourced, it is expected to be compatible with OSS Kibana, instead of a private repo, like opendistro-for-elasticsearch/kibana-oss. We open-source AD in order to get attention from external contributors, and they must be using elastic/kibana or any other public oss kibana.

If it can go public, we can just use public opendistro-for-elasticsearch/kibana-oss, but I think it may involve more legal discussion. We can propose this idea to leadership.

If our UT passes for elastic/kibana, but fail for opendistro-for-elasticsearch/kibana-oss, I would say we should figure out root cause and decide which solution to go. In that case, there must be some breaking changes between elastic/kibana and opendistro-for-elasticsearch/kibana-oss even at same version, it is worth the investigation.

Copy link
Contributor Author

@ohltyler ohltyler Jun 9, 2020

Choose a reason for hiding this comment

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

As stated in the link in the PR description, the problem is no secrets can be passed to the github runner if it is coming from a forked branch. There is a security vulnerability if that was not the case (e.g., someone could put a PR out with an action that saves the contents of the secret to some file).

This is why originally this workflow was only running after pushes to master rather than pull requests from forked branches to master.

Me and @yizheliu-amazon were thinking of adding the ability to test against PRs by running our plugin using the elastic/kibana to avoid setting a secret.

Copy link
Contributor

@ylwu-amzn ylwu-amzn Jun 9, 2020

Choose a reason for hiding this comment

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

Sure, have pinged Alolita about whether we should use elastic/kibana for testing workkfow and whether we can make opendistro-for-elasticsearch/kibana-oss public.

Another work around may be we use elastic/kibana for PR triggered test workflow, and opendistro-for-elasticsearch/kibana-oss for push triggered test workflow. So we can test both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That could also work. Will hold off on merging until you hear back. Thanks for checking

@ylwu-amzn ylwu-amzn mentioned this pull request Jun 9, 2020
@ohltyler ohltyler force-pushed the changeUTWorkflow branch from 99b25ca to 0d0ddf1 Compare June 9, 2020 21:58
@ohltyler
Copy link
Contributor Author

Will close this PR for now. Once a decision is made, will make changes to all workflows in one PR (CD action, UT action)

@ohltyler ohltyler closed this Jun 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change UT workflow to run against new pull requests
3 participants