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

feat: add single lidar sensor kit #4988

Merged

Conversation

beginningfan
Copy link
Contributor

@beginningfan beginningfan commented Jul 15, 2024

Description

Add single lidar sensor kit into autoware.repos

Tests performed

Not applicable.

Effects on system behavior

Not applicable.

Interface changes

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@xmfcx
Copy link
Contributor

xmfcx commented Jul 18, 2024

@youtalk -san, we have an error here:

#37 exporting cache to registry
#37 writing layer sha256:097f051f1338202158d30585f12c3d0a1e9c2340987eb8b76fb6d7daba869831 0.3s done
#37 writing layer sha256:09d0ae49c822b9d412dbacc4fb4c0b4691d094474298503107d86279bff132f5
#37 writing layer sha256:09d0ae49c822b9d412dbacc4fb4c0b4691d094474298503107d86279bff132f5 0.1s done
#37 writing layer sha256:14696ea74c165d8c59b1eeb021fd1456e38b2c6be4b29f6dce2fb0079bebd5ab 0.1s done
#37 writing layer sha256:1c992689c77477160e8b36df6c81ab11335fd44bb7b4d6fa4130eeedcba432d7
#37 writing layer sha256:1c992689c77477160e8b36df6c81ab11335fd44bb7b4d6fa4130eeedcba432d7 0.1s done
#37 writing layer sha256:229e0d614490878cdec2e857c1d3dd706b441e0acad2d6b38c3215daa266ec15
#37 preparing build cache for export 139.4s done
#37 writing layer sha256:229e0d614490878cdec2e857c1d3dd706b441e0acad2d6b38c3215daa266ec15 0.2s done
#37 ERROR: error writing layer blob: unexpected status from POST request to https://ghcr.io/v2/autowarefoundation/autoware-buildcache/blobs/uploads/: 403 Forbidden
------
 > exporting cache to registry:
------
WARNING: No output specified with docker-container driver. Build result will only remain in the build cache. To push result image into registry use --push or to load image into docker use --load

 5 warnings found (use --debug to expand):
 - FromAsCasing: 'as' and 'FROM' keywords' casing do not match (line 4)
 - FromAsCasing: 'as' and 'FROM' keywords' casing do not match (line 37)
 - FromAsCasing: 'as' and 'FROM' keywords' casing do not match (line 74)
 - FromAsCasing: 'as' and 'FROM' keywords' casing do not match (line 113)
 - FromAsCasing: 'as' and 'FROM' keywords' casing do not match (line 130)
ERROR: failed to solve: error writing layer blob: unexpected status from POST request to https://ghcr.io/v2/autowarefoundation/autoware-buildcache/blobs/uploads/: 403 Forbidden

@xmfcx xmfcx force-pushed the feat/add-single-lidar-sensor-kit branch from 7894fb6 to 15bee06 Compare July 18, 2024 21:55
@xmfcx
Copy link
Contributor

xmfcx commented Jul 18, 2024

https://github.com/autowarefoundation/autoware/actions/runs/9999203274/job/27639752909?pr=4988

@youtalk -san, this is still failing even after the rebase. Maybe forks cannot make use the build cache? What can we do?

https://autowarefoundation.github.io/autoware-documentation/main/contributing/pull-request-guidelines/#general-pull-request-workflow

Autoware uses the fork-and-pull model. For more details about the model, refer to GitHub Docs.

If it's failing because of the fork, we should find a way to make it work with them. PRs from forks should be able to pass required workflows.

@youtalk
Copy link
Member

youtalk commented Jul 19, 2024

I see and will conduct a replication experiment to determine the cause.

@youtalk
Copy link
Member

youtalk commented Jul 19, 2024

I could conduct the replication. #5015

I think the github.token is the problem.
https://github.com/autowarefoundation/autoware/blob/main/.github/actions/docker-build/action.yaml#L98

If a PR owner has no writable permission to this repository, they cannot run the CIs.

@xmfcx @mitsudome-r Please make a secret to access the GHCR with writable permission and then register it to the repository's secrets.
https://docs.github.com/en/actions/security-guides/using-secrets-in-github-actions

@xmfcx
Copy link
Contributor

xmfcx commented Jul 19, 2024

@youtalk san, thanks for confirming the issue.

Please make a secret to access the GHCR with writable permission and then register it to the repository's secrets.

Well, public forks cannot access to the upstream repo's secrets due to security concerns.

It is possible to make it with pull_request_target: trigger but it is still unsafe.

https://stackoverflow.com/questions/76975408/how-to-pass-a-secret-from-a-forked-github-repo-to-source-repo


I'd rather not let forks to have write access to the container registry. An attacker could use it to write unwanted images to our registry by just modifying the workflow file.

Why do we allow the cache to be updated when something is not merged yet?

During the PR runs, before the PR is merged, nothing should be written to the cache. What do you think?

@youtalk
Copy link
Member

youtalk commented Jul 19, 2024

During the PR runs, before the PR is merged, nothing should be written to the cache. What do you think?

This is a better alternative. I will do it next weekday. Please wait a while.

@youtalk youtalk enabled auto-merge (squash) July 23, 2024 06:43
Copy link
Member

@youtalk youtalk left a comment

Choose a reason for hiding this comment

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

LGTM
I've updated the branch to resolve the health-check issue.

@youtalk youtalk merged commit 79429d4 into autowarefoundation:main Jul 23, 2024
17 checks passed
youtalk added a commit to youtalk/autoware that referenced this pull request Jul 24, 2024
* feat(ci): disable `cache-to` option to run `health-check` from forked branch (autowarefoundation#5021)

disable cache-to

Signed-off-by: Yutaka Kondo <[email protected]>

* feat: add single lidar sensor kit (autowarefoundation#4988)

feat(autoware.repos): add single lidar sensor kit

Signed-off-by: beginningfan <[email protected]>
Co-authored-by: Yutaka Kondo <[email protected]>

* fix(docker-build): fix ccache typo (autowarefoundation#5024)

Signed-off-by: mitsudome-r <[email protected]>

* feat(docker): no longer download artifacts on `devel` image (autowarefoundation#5023)

not download artifacts on devel

Signed-off-by: Yutaka Kondo <[email protected]>

---------

Signed-off-by: Yutaka Kondo <[email protected]>
Signed-off-by: beginningfan <[email protected]>
Signed-off-by: mitsudome-r <[email protected]>
Co-authored-by: beginningfan <[email protected]>
Co-authored-by: Ryohsuke Mitsudome <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag:run-health-check Run health-check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants