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

Add field to DevWorkspace to allow customizing any pod fields in deployment. #872

Merged
merged 4 commits into from
Sep 19, 2022

Conversation

amisevsk
Copy link
Contributor

What does this PR do?:

Add support for specifying overrides to pod fields in DevWorkspaces. This PR is a draft for discussion.

The added pods field in the DevWorkspace CRD supports all fields copy-pasted from the pod spec itself, omitting containers and initContainers. In the future it may be worth automating the generation of this field from the pod spec directly.

Containers are omitted as

  1. The CRD yaml size is dramatically increased if they are included, as it brings a lot of fields and documentation
  2. It's not clear whether it should be supported, as many of those fields are already in the container component.

This changes adds 100KB to the DevWorkspace CRDs.

Which issue(s) this PR fixes:

Closes #860

PR acceptance criteria:

Testing and documentation do not need to be complete in order for this PR to be approved. We just need to ensure tracking issues are opened.

  • Open new test/doc issues under the devfile/api repo
  • Check each criteria if:
  • There is a separate tracking issue. Add the issue link under the criteria
    or
  • test/doc updates are made as part of this PR
  • If unchecked, explain why it's not needed

How to test changes / Special notes to the reviewer:

@amisevsk amisevsk requested a review from l0rd June 13, 2022 22:53
@amisevsk amisevsk changed the title Pod spec override Add field to DevWorkspace to allow customizing any pod fields in deployment. Jun 14, 2022
@amisevsk amisevsk force-pushed the pod-spec-override branch 2 times, most recently from 411ae8d to 2ebe99d Compare June 16, 2022 21:51
@amisevsk amisevsk marked this pull request as ready for review July 22, 2022 21:40
Copy link
Contributor

@l0rd l0rd left a comment

Choose a reason for hiding this comment

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

I have approved the PR although the pods field could be renamed as discussed in #860 (comment).

@amisevsk
Copy link
Contributor Author

Updated field name to podSpecOverride as suggested. We can decide if it should apply to Kubernetes components down the line.

@openshift-ci openshift-ci bot added the lgtm label Jul 31, 2022
@openshift-ci
Copy link

openshift-ci bot commented Jul 31, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, l0rd

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

Add field `podSpecOverride` to the DevWorkspace spec (but not Devfile or
DevWorkspaceTemplate) that allows specifying arbitrary fields on any
pods created for the DevWorkspace.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Fix potential nil error in generator if attempting to update JSON schema
for a field that has the 'endpoints' property but isn't a
ContainerComponent (e.g. a Pod spec).

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Controller-gen cannot generate embedded metadata fields, resulting in
those fields deserializing to empty objects. In order to embed metadata
in a CRD, it is necessary to duplicate metadata fields where
appropriate.

See issue: kubernetes-sigs/controller-tools#385
for details

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
…ield

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@openshift-ci
Copy link

openshift-ci bot commented Sep 19, 2022

New changes are detected. LGTM label has been removed.

@codecov-commenter
Copy link

Codecov Report

Base: 35.51% // Head: 34.75% // Decreases project coverage by -0.76% ⚠️

Coverage data is based on head (8a26991) compared to base (27328ab).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #872      +/-   ##
==========================================
- Coverage   35.51%   34.75%   -0.77%     
==========================================
  Files          52       52              
  Lines        6653     6799     +146     
==========================================
  Hits         2363     2363              
- Misses       4145     4291     +146     
  Partials      145      145              
Impacted Files Coverage Δ
pkg/apis/workspaces/v1alpha2/devworkspace_types.go 100.00% <ø> (ø)
.../apis/workspaces/v1alpha2/zz_generated.deepcopy.go 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@amisevsk amisevsk merged commit 31045fa into devfile:main Sep 19, 2022
@amisevsk amisevsk deleted the pod-spec-override branch September 19, 2022 17:50
@amisevsk amisevsk mentioned this pull request Sep 28, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow DevWorkspace CR to specify pod spec overrides
4 participants