-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add reusable workflow scripts #307
Conversation
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 is great stuff!!!
I'm still working my way through this, but here are the comments that I have now.
21d09a9
to
6ef73bb
Compare
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.
More comments. Still plowing through everything.
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 think there's a lot of great improvements in the PR. I've left some comments about things that stood out, but I'm still thinking about ways in which we can eradicate the undesirable things that are inherited from the pre-existing scripts.
I think it's good for us to really take that step back and look at this workflow from a high-level, and ask ourselves how we would implement each section given a blank slate. In this case we really do have a blank slate.
A key criteria (for myself) in considering the scripts reusable is for them to be compossible in somewhat arbitrary ways. We are constrained on how granular that decomposition by the logical boundaries of the tasks. I imagine the reusable workflow scripts should allow me to provide input to a script that can carry out any logical task listed in the docs and provide output in a way that could potentially be piped into another script.
COPY --from=test-app-builder /usr/local/bin/summon /usr/local/bin/summon | ||
|
||
#---copy secrets.yml into image---# | ||
COPY tmp.$namespace.secrets.yml /etc/secrets.yml |
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 can be introduced as a configmap.
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.
Let's discuss and create an issue
282ff99
to
001386c
Compare
40a4bcc
to
75690a5
Compare
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.
NOTE: The PR will require squashing the commits to merge since there's been a lot of back and forth and so the commit history isn't necessary made up of logical units.
Approved, with some follow up issues in mind, here's a rough list
- Rip out platform branching
- Revisit platform branching. Helm is great for this,
openshift=true
can be used to toggle between openshift and k8s in the spirit of the bitnami postgres chart - Update policy loading to match dap-wiki (simpler)
- Refactor scripts in a way that allows personas to execute steps independent of each other in a way that aligns with dap-wiki
- Refactor wholesale loading of policy, to allow for loading of policy and writing of variables local to scripts and using the identity of the relevant persona
What does this PR do?
This PR adds end-to-end workflow test scripts, ported over from conjurdemos/kubernetes-conjur-demo
The ported scripts were modified to:
conjur-oss
in a KinD clusterkubernetes-cluster-config
andapp-namespace-prep
The
app-deploy
chart was modified to takeenabled
flags to select one or more subcharts. Theapp-summon-sidecar
subchart is enabled using--set authn-k8s.enabled=true
when installing the parentapp-deploy
chart.The workflow can be tested by launching
bin/test-workflow/start
.Commit 2 (947fb35) copies unchanged files from conjurdemos/kubernetes-conjur-demo (a4272e3)
Commit 3 (51a92d5) is where the script changes are.
What ticket does this PR close?
Resolves #239 and #301
Checklists
Change log
Test coverage
Documentation
README
s) were updated in this PR, and/or there is a follow-on issue to update docs, orManual tests
If you are preparing for a release, have you run the following manual tests to verify existing functionality continues to function as expected?