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

Fix: Add application name to the generated workload entities #3786

Merged
merged 2 commits into from
Apr 29, 2022
Merged

Fix: Add application name to the generated workload entities #3786

merged 2 commits into from
Apr 29, 2022

Conversation

dhiguero
Copy link
Collaborator

Description of your changes

This PR adds the application name to the generated workloads metadata labels (e.g., deployments, jobs, etc) that are created using the standard component types: webservice, worker, task, and cron-task.

The motivation behind this change is to maintain the old behavior of the previous kubernetes runtime where generated entities such as deployments or their resulting pods carried the labels of the parent application and component. That enabled using label selectors to get all pods of an application:

$ k get pods -l=app.oam.dev/name=first-vela-app-as-webservice
NAME                                         READY   STATUS    RESTARTS   AGE
express-server-webservice-54dcccbf4b-qcskl   1/1     Running   0          2m21s

The ability of retrieving pods by application name and/or component is quite convenient for both final users and intermediate tools that rely on that capability.

I have:

  • Read and followed KubeVela's contribution process.
  • Related Docs updated properly. In a new feature or configuration option, an update to the documentation is necessary.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

To test the changes, deploy the updated component definitions, and deploy example applications. For example:

Webservice application
apiVersion: core.oam.dev/v1beta1
kind: Application
metadata:
  name: first-vela-app-as-webservice
spec:
  components:
    - name: express-server-webservice
      type: webservice
      properties:
        image: crccheck/hello-world
        ports:
          - port: 8000
            expose: true

Next create the application and check that the associated pods can be retrieved by component (as before) and by application name.

$ kubectl create -f first-vela-app.webservice.yaml
application.core.oam.dev/first-vela-app-as-webservice created
$ kubectl get pods -l=app.oam.dev/name=first-vela-app-as-webservice
NAME                                         READY   STATUS    RESTARTS   AGE
express-server-webservice-54dcccbf4b-6wdnn   1/1     Running   0          38s
$ kubectl get pods -l=app.oam.dev/component=express-server-webservice
NAME                                         READY   STATUS    RESTARTS   AGE
express-server-webservice-54dcccbf4b-6wdnn   1/1     Running   0          91s

This tests were also executed for the other types of entities.

Special notes for your reviewer

@msftgits
Copy link

msftgits commented Apr 28, 2022

CLA assistant check
All CLA requirements met.

Signed-off-by: Daniel Higuero <daniel@napptive.com>
Signed-off-by: Daniel Higuero <daniel@napptive.com>
@codecov
Copy link

codecov bot commented Apr 28, 2022

Codecov Report

Merging #3786 (90f79e8) into master (7f5b8ef) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3786      +/-   ##
==========================================
+ Coverage   63.99%   64.03%   +0.03%     
==========================================
  Files         312      312              
  Lines       29530    29530              
==========================================
+ Hits        18898    18909      +11     
+ Misses       8183     8176       -7     
+ Partials     2449     2445       -4     
Flag Coverage Δ
apiserver-unittests 35.29% <ø> (-0.01%) ⬇️
core-unittests 56.19% <ø> (+0.09%) ⬆️
e2e-multicluster-test 26.65% <ø> (-0.03%) ⬇️
e2e-rollout-tests 23.61% <ø> (-0.01%) ⬇️
e2etests 31.40% <ø> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/apiserver/sync/cr2ux.go 56.41% <0.00%> (-5.13%) ⬇️
...dev/v1alpha2/application/application_controller.go 83.03% <0.00%> (-1.82%) ⬇️
...ller/core.oam.dev/v1alpha2/application/revision.go 72.36% <0.00%> (-1.51%) ⬇️
pkg/addon/utils.go 72.00% <0.00%> (-1.00%) ⬇️
pkg/workflow/workflow.go 82.07% <0.00%> (+0.31%) ⬆️
pkg/apiserver/rest/usecase/workflow.go 62.89% <0.00%> (+0.73%) ⬆️
pkg/controller/utils/capability.go 80.30% <0.00%> (+0.88%) ⬆️
pkg/workflow/recorder/recorder.go 87.09% <0.00%> (+1.61%) ⬆️
...tepdefinition/workflowstepdefinition_controller.go 71.27% <0.00%> (+2.12%) ⬆️
...ponentdefinition/componentdefinition_controller.go 91.11% <0.00%> (+5.55%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f5b8ef...90f79e8. Read the comment docs.

Copy link
Collaborator

@wonderflow wonderflow left a comment

Choose a reason for hiding this comment

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

thanks

@wonderflow wonderflow merged commit 9642ed9 into kubevela:master Apr 29, 2022
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.

None yet

3 participants