Create nodetasks.IssueCert() #9282
Conversation
k8s-ci-robot
added
size/L
|
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
cncf-cla: yes
| return i.cert, i.key, i.ca | ||
| } | ||
|
|
||
| func (i *IssueCert) AddFileTasks(c *fi.ModelBuilderContext, dir string, name string, caName string, owner *string) { |
There was a problem hiding this comment.
Not something to address now, but we might want to use a struct just so we can name these fields. (Or we could have 3 methods that each build one of the file tasks, then it's easy to set e.g. the owner on them.)
There was a problem hiding this comment.
Most Builders that use certs write them as three files. A few write as two files, omitting the CA. A very few, like the kubelet builder, need to include them in a configuration file.
I do want to keep the common case succinct. I'll see what using the struct looks like.
|
|
||
| func (i *IssueCert) AddFileTasks(c *fi.ModelBuilderContext, dir string, name string, caName string, owner *string) { | ||
| certResource, keyResource, caResource := i.GetResources() | ||
| c.AddTask(&File{ |
There was a problem hiding this comment.
FYI we have EnsureTask which can cope with duplicates (and enforces that the duplicates should be the same ... I guess we're not hitting it though!)
There was a problem hiding this comment.
That would require two Builders to write the same cert to the same file. It would be a bad practice for two independent things to share the same cert. I might use EnsureTask for the CA file, though.
There was a problem hiding this comment.
Sorry - I was really thinking about the directory!
|
|
||
| func (r *TaskDependentResource) Open() (io.Reader, error) { | ||
| if r.Resource == nil { | ||
| return nil, fmt.Errorf("resource opened before it is ready") |
There was a problem hiding this comment.
Nit: we might want to put the task name in here to help us debug if we do hit it - but I'm guessing we don't expect to hit this.
There was a problem hiding this comment.
We don't expect to hit this: it would require a bug where the consumer of the TaskDependentResource fails to declare it as a dependency. I'm thinking of turning this into a panic() as the retrying behavior of the Task scheduler would mask any such bug.
There was a problem hiding this comment.
👍 either way works. klog.Fatalf == panic but makes it easier to add more information!
|
This LGTM - I had some comments / thoughts which I want to make sure you see @johngmyers , but you can choose whether they're worth resolving. So I'm going to add /approve |
k8s-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johngmyers, justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
lgtm
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
Sounds great, thanks @johngmyers - should I rebase #9248 now or are there more we should get in first? (I'm happy to add the testing for this task in #9248, I'm guessing it'll be required now anyway!) |
|
Please proceed with #9248 now. I was planning on doing further cert work on top of both. I also have one refactor depending on this and |
|
/retest Review the full test history for this PR. Silence the bot with an |
4 similar comments
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest |
No description provided.