-
Notifications
You must be signed in to change notification settings - Fork 5
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
SECURESIGN-1476 | Add the Redis backfill job to Ansible collection #101
Conversation
ca2ca28
to
e67ba38
Compare
roles/tas_single_node/templates/manifests/rekor/backfill_redis.sh.j2
Outdated
Show resolved
Hide resolved
Additionally, ideally you could add a workable test for this within |
Im not really sure there is a good way to test this if I am honest, ill take a look, but really we would only be testing to make sure that we can configure it with ansible and thats already done here |
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.
👋 thanks for the PR Jason. I think this is a very good start, but I put some suggestions inline to improve some of the mechanics of this functionality.
roles/tas_single_node/templates/manifests/rekor/backfill_redis.sh.j2
Outdated
Show resolved
Hide resolved
roles/tas_single_node/templates/manifests/rekor/backfill_redis.sh.j2
Outdated
Show resolved
Hide resolved
roles/tas_single_node/templates/manifests/rekor/backfill_redis.sh.j2
Outdated
Show resolved
Hide resolved
roles/tas_single_node/templates/manifests/rekor/backfill_redis.sh.j2
Outdated
Show resolved
Hide resolved
262b902
to
e77eed2
Compare
Marking as draft until an image with my changes becomes available |
7f85533
to
47f2774
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.
Hi, thanks for putting all the additional work in. This looks really good, I just have one minor comment on the config variable we're introducing.
Also a question: Did you try to see if the timer execution works correctly? (e.g. by modifying the timer locally to execute every minute or something like that to actually see it execute repeatedly)
Thinking about this further, I think we should expose the timer configuration as a variable under tas_single_node_backfill_redis
(see my first comment about modifying this variable). This will both allow users to run the job when they want to and allow us to easily test this feature.
I did yes, everything worked as expected :)
Makes sense :) sure thing |
47f2774
to
a2de282
Compare
a66bf73
to
f87c2c6
Compare
f87c2c6
to
3bbb520
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.
LGTM, great job! Thanks.
Pr to add the backfill redis, setting as draft for now as it should only be merged after #100
Upstream pr: sigstore/rekor#2280