-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[FIX] Remove the apt warning #8624
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
Conversation
|
/kind cleanup |
|
@pritidesai how can I make progress with this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution!
The change to apt-get looks good to me, but I'm not sure about adding the workflow_dispatch option here.
.github/workflows/ci.yaml
Outdated
| on: [pull_request] # yamllint disable-line rule:truthy | ||
| on: | ||
| - pull_request | ||
| - workflow_dispatch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For testing the yamllint job, I would prefer if we added a yamllint target to the Makefile and use make yamllint in the workflow here, although the apt-get part would be trick in the Makefile as it would work only on debian based systems. @vdemeester wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I went for the quickest but indeed I agree with you. We may want to split this into an « install yamllint » action and a make yamllint 😇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comments, will make yamlint available for local testing.
As for workflow_dispatch, it is needed to run tests in my fork before I submit a PR. There is another request for this feature #8621
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if you want me remove workflow_dispatch - due to some security or other reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leshikus let's remove the workflow_dispatch for now then 🙃
|
@vdemeester @afrittoli , can we progress with this patch? I was thinking about spending some time working on this project and resolving more serious issues in future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cc @afrittoli @waveywaves
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester 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 |
|
I wonder how can this PR get this LGTM label |
|
@leshikus can you take care of my last comment, and I'll LGTM 👼🏼 😉 |
|
@leshikus I have updated the PR with the required change on your behalf hope that is ok, this is so that we can fast-forward this PR :) cc @vdemeester |
|
@waveywaves thanks! sorry for missing the previous request |
|
/lgtm ing based on Vincent's last comment 🚀 |
Changes
The
aptmanpage recommends against using it in scripts and issues a warning. This patch fixes the warning.It also adds an option to debug ci worlflow via
workflow_dispatch, and removes yamllint exception by properly formatting the on clause.Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes