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 nextcloud compatibility and firebase optionality #978

Closed
wants to merge 2 commits into from
Closed

feat: add nextcloud compatibility and firebase optionality #978

wants to merge 2 commits into from

Conversation

zoop-btc
Copy link
Contributor

@zoop-btc zoop-btc commented May 17, 2022

I would like to propose these changes so it'll be possible to use Nextcloud as a backup mechanism.
This also introduces a new structure to the values. It makes sense to consolidate them as backup since they're both used for the mongodb as well as the scb backup. This is what I came up with but please feel free to give input!

backup:
  googlecloud:
    enabled: true
    bucketName: galoy-staging-backups
    secretName: gcs-sa-key
  dropbox:
    enabled: true
    secretName: dropbox-access-token
  nextcloud:
    enabled: false
    secretName: nextcloud-credentials

If a user wants to deploy the charts right now without having access to dropbox/google cloud/firebase then it will fail since the secrets referenced don't exist. The only solution I saw was to encapsulate them in if statements and use firebaseaccountneeded as well as the enabled values above. I made it so it'll still reflect the defaults by disabling nextcloud. However disabling firebase will turn off the daily notifications, which anybody deciding to forego google needs to be made aware of. Maybe I will tackle that later on.

Are there any other issues which not using google at all could create? (I have not noticed any in my test environment.)

This pull request coincides with one for the galoy repo .

The secret for nexcloud should look like this and needs to be manually applied like the others:

apiVersion: v1
kind: Secret
metadata:
  name: nextcloud-credentials
type: Opaque
data:
  host: <base64encodedfullurl>
  user: <base64encodeduser>
  password: <base64encodedpassword>

The host should point to the full url of the nextcloud webdav target like this:
https://myserver.com/remote.php/dav/files/myuser/backupfolder

thank you kindly for the consideration
ps: sorry adding so many checks to the mongodb backup script, but better safe than sorry

@nicolasburtey
Copy link
Member

thanks for your contribution @zoop-btc !

I didn't dive into the code, but making firebase a non mandatory dependency is very useful

Copy link
Member

@krtk6160 krtk6160 left a comment

Choose a reason for hiding this comment

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

The code involving mongo backup tools can be removed as we have now shifted to using a dedicated docker image for the mongo backup cronjob which has the dependency preinstalled

charts/galoy/templates/mongo-backup-configmap.yaml Outdated Show resolved Hide resolved
charts/galoy/templates/mongo-backup-configmap.yaml Outdated Show resolved Hide resolved
@zoop-btc
Copy link
Contributor Author

Thank you @krtk6160 this looks even better now since the mongodb-tools value can be completely removed :)
I've also cleaned up the pull request, sorry new to github.

@zoop-btc zoop-btc changed the title Nextcloud compatibility & Degoogle feat: add nextcloud compatibility and firebase optionality Jun 6, 2022
@zoop-btc zoop-btc marked this pull request as draft June 6, 2022 12:07
@zoop-btc
Copy link
Contributor Author

zoop-btc commented Jun 7, 2022

I have moved the trigger outside of the deployment loop @bodymindarts as per your suggestion.
This means another change to the values:

trigger:
name: trigger
appEntrypoint: "lib/servers/trigger.js"
healthz: true
targetPort: 8888
replicaCount: 1
timeoutSeconds: 30
service:
type: ClusterIP
targetPort: 8888
port: 8888

This format would be followed by the other galoy deployments. I can do it for the others as well in a separate PR.
I would also like to make the resources configurable there instead of having them fixed in the template.

@krtk6160
Copy link
Member

krtk6160 commented Jun 8, 2022

I have moved the trigger outside of the deployment loop @bodymindarts as per your suggestion. This means another change to the values:

trigger:
name: trigger
appEntrypoint: "lib/servers/trigger.js"
healthz: true
targetPort: 8888
replicaCount: 1
timeoutSeconds: 30
service:
type: ClusterIP
targetPort: 8888
port: 8888

This format would be followed by the other galoy deployments. I can do it for the others as well in a separate PR. I would also like to make the resources configurable there instead of having them fixed in the template.

@sandipndev iirc you too were refactoring the galoy chart. Does that have any overlap/conflict with the changes here? Might make sense to sync with @zoop-btc to avoid duplicated efforts or conflicts

@sandipndev
Copy link
Member

@krtk6160 nope, I was doing the secrets so it's not in the scope of this PR at all.

@zoop-btc
Copy link
Contributor Author

updated branch - no breaking changes found - ready for review

@zoop-btc zoop-btc marked this pull request as ready for review June 16, 2022 12:25
@zoop-btc zoop-btc requested a review from krtk6160 June 17, 2022 06:07
charts/galoy/values.yaml Outdated Show resolved Hide resolved
@zoop-btc zoop-btc requested a review from krtk6160 July 1, 2022 07:03
@krtk6160
Copy link
Member

@zoop-btc There's a significant refactor of the galoy chart that @sandipndev is doing in #1193. It has some amount of overlap with this PR, so those parts will no longer be necessary once we rebase this PR following the merge of #1193.

P.S. No need to rebase now, #1193 is still WIP.

@zoop-btc
Copy link
Contributor Author

@krtk6160 Understood, I'll wait for it to be merged and then adjust this PR accordingly. Thank you for the heads-up.
I'm putting it back to draft mode.

@zoop-btc zoop-btc marked this pull request as draft July 13, 2022 16:42
*refactor: make all google firebase credentials optional
*refactor: make dropbox and google cloud credentials optional
*refactor: consolidate backup mechanism variables
@zoop-btc
Copy link
Contributor Author

closing this, I will split it into two pull requests, one for nextcloud and the other to make firebase optional - I think factoring trigger out of the deployment was too big of a scope

@zoop-btc zoop-btc closed this Aug 19, 2022
@sandipndev
Copy link
Member

@zoop-btc the newer refactored charts have been merged. It now allows you to add in another nextcloud and firebase fields to https://github.com/GaloyMoney/charts/blob/main/charts/galoy/values.yaml#L184 and the overall work would be much cleaner.

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