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.

Sign up for GitHub

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

Jump to bottom

[Automated] Update actions #1061

Merged
merged 1 commit into from
Oct 14, 2020
Merged

[Automated] Update actions #1061

merged 1 commit into from
Oct 14, 2020

Conversation

mattmoor
Copy link
Member

Produced via:

# meta: knative/.github
# main: knative/client
cp $(find "${GITHUB_WORKSPACE}/meta/workflow-templates" -type f -name '*.yaml') \
  "${GITHUB_WORKSPACE}/main/.github/workflows/"

/cc rhuss maximilien
/assign rhuss maximilien

@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 13, 2020
@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 13, 2020
maximilien
maximilien reviewed Oct 13, 2020
View reviewed changes
Copy link
Contributor

@maximilien maximilien left a comment

Choose a reason for hiding this comment

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

/ok-to-test

@knative-prow-robot knative-prow-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 13, 2020
@rhuss
Copy link
Contributor

rhuss commented Oct 13, 2020

Let's discuss our usage of GitHub actions in today's client WG call, but looks good to me.

rhuss
rhuss reviewed Oct 13, 2020
View reviewed changes
Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Looks good to me in general, but I have the feeling that some actions could be combined to reduce the number of checks. Of course, it's a trade-off between being too coarse-grained and having some checks not running that also might fail, and too fine-grained which for sure eat up more resources (more containers need to be spin up, every time the source code needs to be checked out again, reviewdog needs to be download multiples times for every CI run).

As these scripts are homogenous across all Knative repos, this is probably not the proper place to discuss this (?)

Also, some short documentation would be helpful which configuration file a repo can create to tune the quality checks. Maybe a .github/workflows/README.md ?

.github/workflows/knative-go-build.yaml
tr '\n' ' ')"

echo "Building with tags: ${tags}"
go test -vet=off -tags "${tags}" -run=^$ ./... | grep -v "no test" || true
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like that this "Build" step also runs the tests. Maybe the name should reflect this or this command should be changed to go build

On the other hand, I wonder why we shouldn't run our on build.sh which would also do all the testing ?

.github/workflows/knative-go-test.yaml
run: echo 'COVER_OPTS=-coverprofile=coverage.txt -covermode=atomic' >> $GITHUB_ENV

- name: Test
run: go test -race $COVER_OPTS ./...
Copy link
Contributor

Choose a reason for hiding this comment

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

how is this substantially different from the 'build' step above, except that it also check the code coverage ? Maybe both actions could be combined to reduce the overhead ? (and save some trees)

@rhuss
Copy link
Contributor

rhuss commented Oct 13, 2020

Added two issues over there to tackle the issues mentioned above in the review comment:

@mattmoor
Update common github actions
da090bd 
Signed-off-by: Matt Moore (via Sockpuppet) <[email protected]>
@mattmoor mattmoor force-pushed the auto-updates/common-actions branch from 753897d to da090bd Compare October 13, 2020 13:16
@rhuss
Copy link
Contributor

rhuss commented Oct 13, 2020

/retest

@mattmoor mattmoor mentioned this pull request Oct 14, 2020
Closed
@rhuss
Copy link
Contributor

rhuss commented Oct 14, 2020

Let's merge that and see how it goes

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, rhuss

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot merged commit 880aa1a into knative:master Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhuss rhuss rhuss left review comments

@maximilien maximilien maximilien left review comments

Assignees

@maximilien maximilien

@rhuss rhuss

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mattmoor @rhuss @knative-prow-robot @maximilien