-
Notifications
You must be signed in to change notification settings - Fork 707
Fix plugin secrets in config #6249
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
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for nextflow-docs-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Here is how I've been testing:
However, it's not loading the
It's at least loading |
I wonder if the issue is that I'm using the dist release instead of the standard release |
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.
This looks smart. Moving to draft until the problem with the plugins is addressed. Also unit test and integration test should be added (it may be convenience setup a test GH repo for this)
It turns out the issue was that I was using the single-VM CE, which doesn't support AWS secrets. It works with AWS Batch. Regarding testing, I don't think we'll need a git repo unless you want to do an e2e test with platform. I'll add a local e2e test that just tests the detection of config secrets -> config reloading. |
One minor issue with this double-loading approach is that you can't use secrets in a config include (#5312). On the first load, the secret will be null and the include will likely fail, which means the run will fail However, you should be able to work around this using a dynamic include: includeConfig secrets.GITHUB_TOKEN
? "https://raw.githubusercontent.com/markpanganiban/nf-test/master/ubuntu.config?token=${secrets.GITHUB_TOKEN}"
: '/dev/null' On the first load, it will access the secret, triggering a reload, but won't attempt to include the secured config. On the second load it will include the config. I will add this to the docs on config secrets |
e8f25b9
to
5d5ef8a
Compare
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
5d5ef8a
to
f03a097
Compare
The e2e test that I added is failing in GitHub, but not sure why because it works for me locally |
For me does not work locally
|
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
I see now, I wasn't testing it correctly on local. The e2e test runs with |
Fix #5494 #5943
This PR fixes support for plugin secrets (i.e. AWS secrets) in the config by loading the config before resolving plugins. If the config attempts to use secrets, it will be reloaded with the complete set of secrets providers.
This is possible because plugin config must be static, unlike other config which can be dynamic and reference secrets. Therefore, we can simply load the config to extract the list of plugins (and other config settings like
tower.enabled
andprocess.executor
which can trigger additional core plugins.To minimize the impact of loading the config twice, we only reload the config if we detect that secrets were used by the config on the first load. This way, the double config load should only affect users who want to use secrets in the config.
In summary: