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

[AMORO-2938] Add common dependency chart #2939

Merged
merged 2 commits into from
Jun 25, 2024
Merged

Conversation

xleoken
Copy link
Member

@xleoken xleoken commented Jun 18, 2024

Why are the changes needed?

Close #2938.

Brief change log

Due to the network issue, it's very very hard for us to pull the common dependency from registry-1.docker.io. We can add the dependency to the project directly.

# helm dependency build
Error: no repository definition for oci://registry-1.docker.io/bitnamicharts. Please add the missing repos via 'helm repo add'

# ping registry-1.docker.io
PING registry-1.docker.io (54.196.99.49) 56(84) bytes of data.

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before making a pull request

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@czy006
Copy link
Contributor

czy006 commented Jun 18, 2024

We cannot review the contents of the compressed package, which may be risky. Can we specify the pull this common method through http get it ?

@xleoken
Copy link
Member Author

xleoken commented Jun 18, 2024

We cannot review the contents of the compressed package, which may be risky. Can we specify the pull this common method through http get it ?

emm, it seems no other ways. I used gcp to download the dependency.
17abdb54e2c452564174295c9aa70a5

@baiyangtx
Copy link
Contributor

It's not a good idea to add a tgz file to git repo.

Maybe you can build the amoro helm package in an environment where the network allows it and then distribute it to the environment where it needs to be deployed

@baiyangtx
Copy link
Contributor

Or Amoro can maintain built helm charts.
Currently, the Amoro project provides packaged charts in gh-pages, but this branch is not maintained.
Perhaps you can add a workflow to maintain this distribution

@xleoken
Copy link
Member Author

xleoken commented Jun 18, 2024

It's not a good idea to add a tgz file to git repo.

Maybe you can build the amoro helm package in an environment where the network allows it and then distribute it to the environment where it needs to be deployed

ok, let me think again.

@xleoken xleoken force-pushed the chart branch 3 times, most recently from 8e3d69c to 3d0f5cf Compare June 18, 2024 07:01
@xleoken
Copy link
Member Author

xleoken commented Jun 18, 2024

@daragu
Copy link
Contributor

daragu commented Jun 18, 2024

This problem has been bothering me for a long time, nice to see that this issue is resolving.

@kristgpt
Copy link

It's also difficult for me to download the common dependency.

Copy link
Contributor

@baiyangtx baiyangtx left a comment

Choose a reason for hiding this comment

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

Is it possible to put these files into a separate directory?

@xleoken
Copy link
Member Author

xleoken commented Jun 21, 2024

Is it possible to put these files into a separate directory?

Good idea, updated.

image

Copy link
Contributor

@baiyangtx baiyangtx 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 for your contributions

@zhoujinsong zhoujinsong merged commit dd50583 into apache:master Jun 25, 2024
5 checks passed
@zhoujinsong
Copy link
Contributor

Thanks for the work! @xleoken
Thanks for the review! @baiyangtx

@xleoken xleoken deleted the chart branch June 28, 2024 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement]: Add common dependency chart
6 participants