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 DevfileOptions to GetPodTemplateSpec #167

Merged
merged 3 commits into from
Mar 23, 2023

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Mar 9, 2023

What does this PR do?:

The PR adds the ability to filter the containers included in the generator.PodTemplateSpec() when calling GetPodTemplateSpec. This is equivalent to filtering when calling GetContainers()

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:

@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Patch coverage: 69.14% and project coverage change: +0.05 🎉

Comparison is base (539d4b1) 60.41% compared to head (0266b32) 60.47%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #167      +/-   ##
==========================================
+ Coverage   60.41%   60.47%   +0.05%     
==========================================
  Files          36       37       +1     
  Lines        4229     4369     +140     
==========================================
+ Hits         2555     2642      +87     
- Misses       1525     1567      +42     
- Partials      149      160      +11     
Impacted Files Coverage Δ
pkg/devfile/parser/data/v2/header.go 100.00% <ø> (ø)
pkg/devfile/parser/data/v2/projects.go 94.73% <ø> (ø)
pkg/devfile/parser/sourceAttribute.go 85.91% <ø> (ø)
pkg/util/util.go 38.69% <ø> (ø)
pkg/devfile/generator/policy.go 64.10% <64.10%> (ø)
pkg/devfile/generator/generators.go 64.78% <72.63%> (-0.45%) ⬇️
pkg/devfile/generator/utils.go 90.90% <100.00%> (+0.28%) ⬆️

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: Philippe Martin <phmartin@redhat.com>
@feloy feloy force-pushed the getpodtemplatespec-options branch from 561a872 to 4c562a1 Compare March 20, 2023 08:08
Copy link
Contributor

@kim-tsao kim-tsao left a comment

Choose a reason for hiding this comment

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

Can we update the copyright year in the generators file to 2022-2023

@@ -238,6 +238,7 @@ func GetDeployment(devfileObj parser.DevfileObj, deployParams DeploymentParams)
// PodTemplateParams is a struct that contains the required data to create a podtemplatespec object
type PodTemplateParams struct {
ObjectMeta metav1.ObjectMeta
options common.DevfileOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

Is options only intended to be used within the generator package and that's why it's private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it should be public. Thanks for the catch

func GetPodTemplateSpec(devfileObj parser.DevfileObj, podTemplateParams PodTemplateParams) (*corev1.PodTemplateSpec, error) {
containers, err := GetContainers(devfileObj, common.DevfileOptions{})
containers, err := GetContainers(devfileObj, podTemplateParams.options)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get additional test coverage for this?

Signed-off-by: Philippe Martin <phmartin@redhat.com>
@feloy feloy force-pushed the getpodtemplatespec-options branch from ac38a51 to 0af08e8 Compare March 23, 2023 07:34
@feloy
Copy link
Contributor Author

feloy commented Mar 23, 2023

Thanks @kim-tsao for your review

@feloy feloy requested review from kim-tsao and removed request for yangcao77 and maysunfaisal March 23, 2023 07:53
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Copy link
Contributor

@kim-tsao kim-tsao left a comment

Choose a reason for hiding this comment

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

Thanks @feloy , the changes look good

@openshift-ci
Copy link

openshift-ci bot commented Mar 23, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feloy, kim-tsao

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kim-tsao kim-tsao merged commit d36e409 into devfile:main Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants