-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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] Remove the apt warning #8624
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/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.
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
Changes
The
apt
manpage 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