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

Feature/image state update action #49

Merged
merged 3 commits into from
Jun 21, 2023

Conversation

semmet95
Copy link
Contributor

@semmet95 semmet95 commented Jun 8, 2023

Description of your changes

Adds trigger-action definition and a corresponding TriggerSerivce example.

I have:

  • Read and followed KubeVela's contribution process.
  • Add related tests.
  • 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

After applying the trigger-action definition and the TriggerService example, respectively, the TriggerService will monitor all the kpack Image type resources on the cluster and deploy a Job as an action when any Image is successfully rebased. The Job's pod will log the name and the namespace of the source Image that triggered the action.

Special notes for your reviewer

The definition can be applied without any issues but creating the TriggerService would fail if kpack is not installed on the cluster.

@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Patch coverage: 75.80% and project coverage change: +0.11 🎉

Comparison is base (2d521b3) 73.19% compared to head (a6fbe7c) 73.30%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #49      +/-   ##
==========================================
+ Coverage   73.19%   73.30%   +0.11%     
==========================================
  Files          13       15       +2     
  Lines        1134     1195      +61     
==========================================
+ Hits          830      876      +46     
- Misses        262      275      +13     
- Partials       42       44       +2     
Flag Coverage Δ
unittests 73.30% <75.80%> (+0.11%) ⬆️

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

Impacted Files Coverage Δ
pkg/source/builtin/cronjob/cronjob.go 64.28% <64.28%> (ø)
pkg/executor/executor.go 89.86% <100.00%> (ø)
pkg/source/builtin/cronjob/config.go 100.00% <100.00%> (ø)

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

@semmet95 semmet95 force-pushed the feature/image-state-update-action branch from 688a7e8 to 5c65348 Compare June 8, 2023 01:54
action:
type: image-update-task
properties:
cmd: [/bin/sh, -c, "echo Image: ${SOURCE_NAME} in namespace: ${SOURCE_NAMESPACE} has been successfully rebased at $(date)"]
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the contribution!
But I don't quite get the usage of this image-update-task job. It seems like in your example, this action will only echo a message in a created job, am I understanding it correctly?

Copy link
Contributor Author

@semmet95 semmet95 Jun 8, 2023

Choose a reason for hiding this comment

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

Yes, I just added added a simple echo command as an example 😬
The image-update-task definition is the more important part.
I could update the example if you have any suggestions though.
The intention is to have some sort of action taken when an image is rebased, since the change in the Image resource itself can be triggered by change in another resource like ClusterStack

Copy link
Member

Choose a reason for hiding this comment

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

I see! In general, the image-update-task will just use job to handle some tasks. Why not we change the name from image-update-task to task?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that makes sense. I was so caught up in using it to react to Image resources that I didn't see its actually a generic action. I'll update the name.

Copy link
Contributor Author

@semmet95 semmet95 Jun 13, 2023

Choose a reason for hiding this comment

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

@FogDong Updated the file and the definition name. Thanks for the suggestion :)

@semmet95 semmet95 requested a review from FogDong June 9, 2023 12:18
semmet95 added 3 commits June 13, 2023 22:46
Signed-off-by: Amit Singh <singhamitch@outlook.com>
Signed-off-by: Amit Singh <singhamitch@outlook.com>
Signed-off-by: Amit Singh <singhamitch@outlook.com>
@semmet95 semmet95 force-pushed the feature/image-state-update-action branch from e083f56 to a6fbe7c Compare June 13, 2023 17:16
@FogDong FogDong merged commit ac10e57 into kubevela:main Jun 21, 2023
@semmet95 semmet95 deleted the feature/image-state-update-action branch June 27, 2023 01:50
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.

2 participants