Skip to content
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

Init containers #188

Merged
merged 5 commits into from
Nov 9, 2020
Merged

Init containers #188

merged 5 commits into from
Nov 9, 2020

Conversation

amisevsk
Copy link
Collaborator

What does this PR do?

Add basic functionality for separating out initContainers from regular components in a devfile, based on the rules defined in devfile/api#32

What issues does this PR fix or reference?

Resolves #183, except code is not used anywhere yet -- it's not clear how to support this with subresources, as the process requires reading the whole devfile. Final implementation will require changing the internal API.

Is it tested? How?

Test cases are written but it is just library code for now.

Additional Info

This PR is targetting the update-sdk-1.x branch used in PR #187, since otherwise there will be some hassle in rebasing.

return &command, nil
}
}
return nil, fmt.Errorf("no command with key %s is defined", id)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future I assume that these rules should be implemented in the validation package of the devfile/api repo.
As soon as we have something working here, we should sync with other teams to move this into the common validation code.

continue
default:
// How a prestart exec command should be implemented is undefined currently, so we reject it.
return fmt.Errorf("only apply-type commands are supported in the prestart lifecycle binding")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future I assume that these rules should be implemented in the validation package of the devfile/api repo.
As soon as we have something working here, we should sync with other teams to move this into the common validation code.

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Nothing to comment apart of minor Key vs ID or id -> ID stuff

Here is a commit with changes you may consider to apply f985c57
I've made them just to understand code better.

pkg/library/lifecycle.go Outdated Show resolved Hide resolved
pkg/library/lifecycle.go Outdated Show resolved Hide resolved
pkg/library/command.go Outdated Show resolved Hide resolved
pkg/library/lifecycle.go Outdated Show resolved Hide resolved
pkg/library/lifecycle.go Show resolved Hide resolved
pkg/library/lifecycle.go Show resolved Hide resolved
pkg/library/command.go Outdated Show resolved Hide resolved
pkg/library/command.go Show resolved Hide resolved
pkg/library/testdata/lifecycle/prestart_exec_command.yaml Outdated Show resolved Hide resolved
Add basic library function that will extract initContainers from a
devfile based on commands and events.

Signed-off-by: Angel Misevski <[email protected]>
@openshift-ci-robot
Copy link
Collaborator

@amisevsk: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

As suggested by David, do not support preStart Exec-type commands in
devfiles and instead return an error.

Signed-off-by: Angel Misevski <[email protected]>
Previous yaml package gopkg.in/yaml.v2 does not support json tags on
fields. Switch to a yaml package that can correctly
serialize/deserialize devfile/api objects.

Signed-off-by: Angel Misevski <[email protected]>
@amisevsk amisevsk changed the base branch from update-sdk-1.x to master November 9, 2020 21:00
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, davidfestal, JPinkney, sleshchenko

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:
  • OWNERS [JPinkney,amisevsk,sleshchenko]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@amisevsk amisevsk merged commit 2b7fa7b into devfile:master Nov 9, 2020
@amisevsk amisevsk deleted the initContainers branch February 8, 2023 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement apply of container component on preStart events
5 participants