-
Notifications
You must be signed in to change notification settings - Fork 1
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 sync functionality #1
base: main
Are you sure you want to change the base?
Conversation
action.yml
Outdated
service_account_file = $HOME/credentials.json | ||
project_number = ${{ secrets.DOCS_GOOGLE_CLOUD_PROJECT_NUMBER }} | ||
bucket_policy_only = true | ||
EOF | ||
|
||
echo ${{ inputs.credentials }} | base64 -d > $HOME/credentials.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, I think this should be an env variable that we set in the action. See my other comment about using secrets in conditions which also mentions some stuff about using secrets w/ CLI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given this is redacted as shown in #1 (comment) is this fine as is or should I still modify it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub recommends using environment variables whenever possible here. I think there's a potential concern if the action runners were compromised or had bad actors they could potentially steal info out of the system monitor since the actual process will print w/ the values on the host machine.
I think you can simplify this a bit by instead using service_account_credentials
which you can conveniently set w/ the RCLONE_GCS_SERVICE_ACCOUNT_CREDENTIALS
environment variable (docs here). So set that env variable in the action to ${{ inputs.credentials }}
and I think you're good (make sure it still obscures the secret in action logs, but I don't know why it wouldn't).
There are also env variables for the other secrets on that docs page like the project number we could set.
You could also use rclone config
CLI instead of making the file. Docs. I think the command would be rclone config create docs "google cloud storage" bucket_policy_only true
. As long as the 2 env variables are set for project number and service account credentials, then it seems to make the config locally for me and it should read the env variables at runtime instead of writing to the file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just went ahead and moved everything to environment variables and it seems to work. The only exception is the credential because it has to base64 decoded and I don't think there's a cleaner way to do that. Considering github does this in an example I think it should be fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few small things. Looking good overall
destination: | ||
required: true | ||
type: string | ||
description: 'The destination directory to sync. Relative to the bucket.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have anything to do with the URL we will generate? Or does it need to include the git repository as the prefix? I see the example has deephaven/deephaven.io/blog
so I'm curious what the importance of deephaven/deephaven.io
is there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure how the bucket is linked to the website, but this is just a location within the bucket. deephaven/deephaven.io/blog
was just where Don wanted the blog content in this case.
Initial version of the sync functionality, tested on my fork