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

Datadir automatic sync #126

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jemacchi
Copy link

@jemacchi jemacchi commented Jan 6, 2025

No description provided.

@jemacchi jemacchi requested a review from edevosc2c January 6, 2025 20:28
@jemacchi jemacchi marked this pull request as draft January 6, 2025 20:28
@@ -393,6 +406,11 @@ rabbitmq:
# storage_class_name: default
# size: 1Gi

gitCredentials:
Copy link
Member

Choose a reason for hiding this comment

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

metadata:
namespace: {{ .Release.Namespace }}
name: {{ include "georchestra.fullname" . }}-datadirsync-role
rules:
Copy link
Member

@edevosc2c edevosc2c Jan 14, 2025

Choose a reason for hiding this comment

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

Would it be possible to limit the roles to the deployments listed in deploymentSuffixNameList?

According to stackoverflow you can limit the access to certain deployments using resourceNames: https://stackoverflow.com/a/68980720

rules:
- apiGroups: ["apps"]
resources: ["pods", "deployments"]
verbs: ["get", "list", "patch", "update"]
Copy link
Member

Choose a reason for hiding this comment

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

Why can't we limit ourselves to just: verbs: ["get", "patch"]?
https://stackoverflow.com/a/68980720

- geoserver
- mapstore
- header
gitCredentials:
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain what's the diff here with the other gitCredentials section in the YAML?

@@ -263,6 +263,19 @@ georchestra:
# relay_username: aaaa
# relay_password: aaaa
extra_environment: []
datadirsync:
enabled: true
Copy link
Member

Choose a reason for hiding this comment

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

Please put the datadir sync disabled by default.

enabled: true
image: jemacchi/simple-git-rollout-operator:1.3.1 # TODO: image should be in context of c2c dockerhub account (?)
pollInterval: 10
deploymentSuffixNameList:
Copy link
Member

Choose a reason for hiding this comment

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

One component is sufficient for the example. You can just limit to - geoserver

@edevosc2c
Copy link
Member

Hello jose, thank you for your contribution.

I have reviewed your PR. Could you also please put a description to your PR so that other contributors understand what is it? You can also reference that it will close #75

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.

2 participants