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

[WIP] Call the workflow from eden #3364

Closed
wants to merge 2 commits into from
Closed

[WIP] Call the workflow from eden #3364

wants to merge 2 commits into from

Conversation

yash-zededa
Copy link
Collaborator

This PR will call the test workflow from eden. The configurations for the github actions would be DRY in the case for eden tests.

Signed-off-by: yash-zededa <yash@zededa.com>
@uncleDecart
Copy link
Contributor

uncleDecart commented Jul 19, 2023

PR for this workflow in EDEN is here lf-edge/eden#877
(Don't merge until this one is merged)

---
name: Eden Test from lf-edge/eden

on:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible with these reusable workflows to specify that smoke tests (job: smoke) should be run already on opening PR, while other tests require approval?
I think it would be great improvement over the current setup, because now even for basic eden tests we have to wait for somebody to approve PR. @uncleDecart WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

@milan-zededa

TL;DR In theory we can. But it could be tricky and will look like magic outside Hogwarts

So we will need to actually create reusable workflow running smoke tests and another running all other tests and condition it on running after successful completion of smoke tests. This raises couple of questions:

  • Will it be run after approval for first time committers? Most likely, yes. Otherwise people can ddos you with random PRs triggering tests which will use up all your resources
  • Will there be problems with LTS releases? Possibly. Right now we are having troubles with assets.yml being run from master branch on LTS. It uses same mechanism I described for smoke tests. This could be a problem if we change tests. We can solve latter with reusable workflows for assets.yml, but conditioning on jobs are a lil bit tricky. We can use something like,
      - name: Network test
        if: ${{ github.event_name == 'push' && github.ref == 'refs/heads/main' }}
        uses: ....

but it might give us overall green light when tests are skipped

Copy link
Contributor

Choose a reason for hiding this comment

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

In any case it would great to look for a way for long-term committers being able to start eden tests on their PRs (at least the smoke ones). Finding out that your PR breaks some essential stuff after review is not ideal. Same for asking someone else to give you approval without looking at the code just to trigger tests.

Copy link
Contributor

@milan-zededa milan-zededa Jul 19, 2023

Choose a reason for hiding this comment

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

Regarding DoS - I don't think there would be any difference between smoke tests job and say PR build job.
And first time committers only get DCO check without approval I believe, so this should not be a problem.

@yash-zededa yash-zededa changed the title Draft: to call the workflow from eden [WIP] Call the workflow from eden Jul 20, 2023
@uncleDecart
Copy link
Contributor

Eden PR is merged, I started a job in eden to see that test.yml works as expected. Once it's completed, we can merge reusable tests, try to run them automatically and then open final PR to switch from eden.yml to eden_test.yml


jobs:
call_test_wf_from_eden:
uses: lf-edge/eden/.github/workflows/test.yml@master
Copy link
Contributor

Choose a reason for hiding this comment

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

How does eden know that it should run with EVE built for a given PR?
I mean this and this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually this is what I wanted to discuss. There's eden version and eve version as well. We need to test given EVE version with specific eden. I believe we need to add environment variable tag to specify EVE version and create versioning for eden (or at least versioning for test.yml action from eden) then when we bumping up eden version we are changing tag and EVE version will be chosen automatically.

@uncleDecart
Copy link
Contributor

uncleDecart commented Jul 24, 2023

@milan-zededa regarding your comment on propagating EVE version into this workflow. We created another PR in eden
lf-edge/eden#878
Which is addressing this concerns, will update this PR with changed input parameters.
Also regarding eden versioning, as of now it'll work if we use master, but next step would be properly defining eden versioning and reusable workflow versioning.

@yash-zededa yash-zededa closed this by deleting the head repository Jul 24, 2023
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.

3 participants