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

ci: move from drone ci to github actions #44

Merged
merged 1 commit into from
May 8, 2024

Conversation

brandboat
Copy link
Member

@brandboat brandboat commented May 2, 2024

@brandboat brandboat self-assigned this May 2, 2024
@brandboat brandboat requested review from bk201 and Yu-Jack May 2, 2024 10:21
Copy link

@Yu-Jack Yu-Jack left a comment

Choose a reason for hiding this comment

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

Cause original drone needs arm64 and amd64, so you need to change following files:

  • scripts/build
  • scripts/package
  • package/Dockerfile*

You could refer other finished repositories, but feel free to ask any question, there might be something we didn't consider.

@brandboat brandboat marked this pull request as draft May 6, 2024 02:48
@brandboat brandboat force-pushed the HARV-5327 branch 2 times, most recently from e4e044b to a402e39 Compare May 6, 2024 06:09
@brandboat brandboat marked this pull request as ready for review May 6, 2024 06:09
@brandboat
Copy link
Member Author

Thanks @Yu-Jack, I've addressed the comments. Please take a look when you are available.

@Yu-Jack Yu-Jack self-requested a review May 7, 2024 04:09
scripts/package Outdated
Comment on lines 10 to 12
if [ -e ${DOCKERFILE}.${ARCH} ]; then
DOCKERFILE=${DOCKERFILE}.${ARCH}
fi
Copy link

Choose a reason for hiding this comment

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

I think we could remove this?

Comment on lines 9 to 11
if [ -e ${DOCKERFILE}.${ARCH} ]; then
DOCKERFILE=${DOCKERFILE}.${ARCH}
fi
Copy link

Choose a reason for hiding this comment

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

same here

Copy link

@Yu-Jack Yu-Jack left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@FrankYang0529 FrankYang0529 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

with:
secrets: |
secret/data/github/repo/${{ github.repository }}/dockerhub/rancher/credentials username | DOCKER_USERNAME ;
secret/data/github/repo/${{ github.repository }}/dockerhub/rancher/credentials password | DOCKER_PASSWORD
Copy link
Member

Choose a reason for hiding this comment

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

Not sure whether we need to change this like
https://github.com/harvester/terraform-provider-harvester/blob/4e07ed02cba6fe7fc37bc0d41ff433df81e69915/.github/workflows/build.yml#L58-L59.

We can merge it first and see whether we can get credential. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure which one is right...
secret/data/github/repo/${{ github.repository }}/dockerhub/rancher/credentials or secret/data/github/repo/${{ github.repository }}/dockerhub/harvester/credentials
I saw some of repos like load-balancer-harvester, vm-import-controller use the former one.
Maybe both of them works.

Copy link
Member Author

Choose a reason for hiding this comment

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

c.c. @Yu-Jack

Copy link

Choose a reason for hiding this comment

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

I asked this question before here https://github.com/rancherlabs/eio/issues/2153#issuecomment-2074150242. I think rancher is enough although I'm not sure why harvester works in https://github.com/harvester/terraform-provider-harvester.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Thanks for the information.

Copy link
Member

@bk201 bk201 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@bk201 bk201 merged commit 93b8872 into harvester:master May 8, 2024
4 checks passed
@brandboat
Copy link
Member Author

@Mergifyio backport v0.1.x v0.2.x

Copy link

mergify bot commented May 8, 2024

backport v0.1.x v0.2.x

✅ Backports have been created

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.

4 participants