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

fix 2 for xlink local cmake #301

Merged
merged 1 commit into from
Dec 14, 2021

Conversation

diablodale
Copy link
Contributor

I ran four sets of tests -- the 2x2 matrix of

  • build shared = yes/no
  • xlink local = path/nothing

And for each test, I verified:
config works
build works
install works
app build works
app works

These tests were run on Windows 10 with VS2019.

@themarpe , I now realize that the CI didn't automatically run when I opened my last PR -- so I didn't get feedback of CI failure so I could fix the issue. CI didn't run until you triggered something.
Is this your team's policy? Or a config issue that my PR was based to develop? As a contributor sample set of 1 😉 it is useful for the CI to give me feedback on my PR so I can fix issues before your team engages.

@diablodale
Copy link
Contributor Author

I see above... it thinks I am a first timer. 🙃
image

@themarpe
Copy link
Collaborator

@themarpe , I now realize that the CI didn't automatically run when I opened my last PR -- so I didn't get feedback of CI failure so I could fix the issue. CI didn't run until you triggered something.
Is this your team's policy? Or a config issue that my PR was based to develop? As a contributor sample set of 1 wink it is useful for the CI to give me feedback on my PR so I can fix issues before your team engages.

Its been lately bumped to require explicit approval - because of HIL testing is being added. Not sure if possible to limit to specific workflows though (as suggested by warning message). That would be better, to still run CI for parts that can be done by public GH runners. Currently there aren't great options in this regard apart from fork solution (that has to be manually updated to keep up to date...)

Copy link
Collaborator

@themarpe themarpe left a comment

Choose a reason for hiding this comment

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

Thanks!

@themarpe themarpe merged commit 7db241e into luxonis:develop Dec 14, 2021
@diablodale
Copy link
Contributor Author

Its been lately bumped to require explicit approval

I'm a gihub actions novice. Could you have a workflow yaml specifically for PRs to develop...

on:
  pull_request:
    branches:
      - develop
runs-on: [win, linux, x64, gpu] // but don't list self-hosted

because all self-hosted runners have the self-hosted label. So also put all your runners except the self-hosted?
Or you have a label lux_hardware and this specific workflow (which is only for PRs to develop) doesn't list that label.

or somehow make workflows that use self-hosted runners need write permissions? because, by default, the github permission token for PRs has only read permission?

@themarpe
Copy link
Collaborator

Right now it seems like the approval is a repository wide setting so it determines behavior for all workflows.
I think what might work a bit better is to move HIL testing into a separate workflow that isn't triggered on PRs.
But still not the best option, as its nice being able to run PRs as long as code is trusted in HIL.
Maybe a combination of both and some bot to trigger the HIL testing with a command from a trusted user for PRs...

Right now the explicit approval covers both so as long as we get to PRs in time to check and run the workflow it'll do for the initial HIL testing iteration IMO.

Major problem with self hosted runner is that even GH didn't yet decide upon a good means of "security" regarding what/who can run on them, etc.. so they straight up recommend not using them in a public repository. For a repo which deals with HW, this isn't the best solution hah

@diablodale
Copy link
Contributor Author

Got it. I was looking around (as I might setup workflows myself). I might investigate something like the far under to trigger a specific workflow based on a specific review comment...

The auto CI can keep your team busy elsewhere while people like me submit a well-intending PR, but missed something (like I must have in my first PR) in our self-test matrix (or don't have the capability for a broad CI matrix). I get your team is evolving, no worries. 👍

# not syntatically correct...
on:
  pull_request_review_comment:
    branches:
      - develop
if: contains(event.comment, '#do-hil') && event.sender.type = 'Collaborator'
runs-on: [lux_hardware]
steps: ...

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.

None yet

2 participants