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

Adding charts ezd-backend and ezd-crd for ezd-rp from Linux Polska #963

Merged
merged 2 commits into from
Feb 21, 2024
Merged

Adding charts ezd-backend and ezd-crd for ezd-rp from Linux Polska #963

merged 2 commits into from
Feb 21, 2024

Conversation

dariuszbolkowski
Copy link
Contributor

Our package contains set of necessary services which allow to lunch EZD-RP application developed by NASK on Kubernetes using Helm.

Pull request contains two helm charts:

  • ezd-crd
  • ezd-backend

@recena
Copy link
Collaborator

recena commented Jan 24, 2024

Since there is a Helm Charts Index Repository (https://linuxpolska.github.io/ezd-rp/index.yaml), you can configure it in Rancher Manager and install the Helm Charts.

📚 Rancher Manager documentation to do it.

@recena recena removed the request for review from atrendafilov February 7, 2024 22:11
@nicholasSUSE
Copy link
Contributor

nicholasSUSE commented Feb 20, 2024 •
edited by recena
Loading

Our package contains set of necessary services which allow to lunch EZD-RP application developed by NASK on Kubernetes using Helm.

Pull request contains two helm charts:

  • ezd-crd
  • ezd-backend

Hello, @dariuszbolkowski

Please provide a screenshot of a working installation of your Chart in Rancher.

I tried here on my end and could not install it:

image

Did you follow this procedure?

📔 Also, your chart needs an initial configuration by entering values for Redis, Postgres etc... This is not a PR blocker, it would be good to have this information in your README.md file for your users.

@dariuszbolkowski
Copy link
Contributor Author

Hi @nicholasSUSE ,

Firstly you need to install operators which is "ezd-crd" package. After that you will be able to install "ezd-backend" package.

Best regards,
Dariusz Bółkowski

@nicholasSUSE
Copy link
Contributor

Hi @nicholasSUSE ,

Firstly you need to install operators which is "ezd-crd" package. After that you will be able to install "ezd-backend" package.

Best regards, Dariusz Bółkowski

Hello @dariuszbolkowski
Your PR looks good to me.
I will be approving.

Just another suggestion:
Normally the charts come configured with the 1st option defined here: https://helm.sh/docs/chart_best_practices/custom_resource_definitions/

There is no problem having to install the CRDs separately first, this is just user experience.

I have tested your chart on Rancher 2.9 and K8s 1.26 with the crds dir.
It worked.

nicholasSUSE
nicholasSUSE previously approved these changes Feb 20, 2024
View reviewed changes
@recena
Copy link
Collaborator

recena commented Feb 20, 2024

@dariuszbolkowski Could you force an empty commit?

@nicholasSUSE
Copy link
Contributor

nicholasSUSE commented Feb 20, 2024 •
edited by recena
Loading

@dariuszbolkowski Could you force an empty commit?

Agreed, I have just tested with a dummy Pull Request and the CI ran successfully.

@dariuszbolkowski Please do a force commit.

recena
recena requested changes Feb 20, 2024
View reviewed changes
packages/linuxpolska/ezd-backend/upstream.yaml
DisplayName: LP Backend for EZD RP
ChartMetadata:
kubeVersion: '>=1.19-0'
icon: https://linuxpolska.com/logo/LinuxPolska-icon.png
Copy link
Collaborator

Choose a reason for hiding this comment

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

@dariuszbolkowski You should add this information:

ChartMetadata:
  maintainers:
  - name: Linux Polska
    email: [email protected]
    url: https://linuxpolska.com/en/

Copy link
Collaborator

Choose a reason for hiding this comment

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

In both helm charts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@recena Will do this shortly. Thanks.

Copy link
Contributor Author

@dariuszbolkowski dariuszbolkowski Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It is done now. Pleasy veryfi.
Let me know if push of empty commit is needed.
Thank you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a conflict with the rebase. Could you do it manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have done rebase. I hope it will works.

@dariuszbolkowski dariuszbolkowski requested a review from a team as a code owner February 21, 2024 06:48
@recena recena merged commit 7e84d73 into rancher:main-source Feb 21, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@recena recena recena approved these changes

@nicholasSUSE nicholasSUSE nicholasSUSE left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dariuszbolkowski @recena @nicholasSUSE