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 CronWorkflow.update, add unit tests #681

Merged
merged 2 commits into from
Jun 15, 2023

Conversation

iameskild
Copy link
Contributor

@iameskild iameskild commented Jun 14, 2023

Pull Request Checklist

Description of PR
Include methods (update and get) to the CronWorkflow object - to match WorkflowTemplate.

cc @elliotgunton

Copy link
Collaborator

@elliotgunton elliotgunton left a comment

Choose a reason for hiding this comment

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

Great stuff, thank you @iameskild! 🚀

@elliotgunton
Copy link
Collaborator

Hey @iameskild - you'll need to rebase and push with signoff to pass the DCO check. You can run these to fix it:

git rebase HEAD~1 --signoff
git push --force-with-lease

@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Merging #681 (8ade723) into main (027b5e2) will increase coverage by 0.0%.
The diff coverage is 72.0%.

❗ Current head 8ade723 differs from pull request most recent head 613844a. Consider uploading reports for the commit 613844a to get more accurate results

@@          Coverage Diff          @@
##            main    #681   +/-   ##
=====================================
  Coverage   76.1%   76.2%           
=====================================
  Files         45      45           
  Lines       3088    3105   +17     
  Branches     584     584           
=====================================
+ Hits        2352    2368   +16     
- Misses       566     567    +1     
  Partials     170     170           
Impacted Files Coverage Δ
src/hera/workflows/cron_workflow.py 83.7% <72.0%> (+6.7%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: iameskild <eskild.eriksen122@gmail.com>
@iameskild
Copy link
Contributor Author

Thanks @elliotgunton! Added my sign-off to the commit :)

@elliotgunton elliotgunton added semver:patch A change requiring a patch version bump type:enhancement A general enhancement labels Jun 15, 2023
@elliotgunton elliotgunton enabled auto-merge (squash) June 15, 2023 12:38
@elliotgunton elliotgunton merged commit e0121d3 into argoproj-labs:main Jun 15, 2023
@iameskild iameskild deleted the update_cronworkflow branch June 15, 2023 12:41
@elliotgunton elliotgunton added semver:minor A change requiring a minor version bump and removed semver:patch A change requiring a patch version bump labels Jun 15, 2023
@iameskild iameskild mentioned this pull request Jun 16, 2023
4 tasks
elliotgunton pushed a commit that referenced this pull request Jun 16, 2023
**Pull Request Checklist**
- [x] Fixes #<!--issue number goes here-->
- [x] Tests added
- [ ] Documentation/examples added
- [x] [Good commit messages](https://cbea.ms/git-commit/) and/or PR
title

**Description of PR**
In my previous PR #681, I'm using the wrong parameter when calling
`UpdateCronWorkflowRequest`; it should've been `cron_workflow` and not
`template`.


To validate that this change is needed, I used this simple example.
Without the fix from this PR, you will likely receive the following
error message:

```
hera.exceptions.InternalServerError: Server returned status code 500 with message: `runtime error: invalid memory address or nil pointer dereference`
```

<details>

```py
import time
from hera.workflows import CronWorkflow, Container

# authenticate w Argo Server
global_config = authenticate()

with CronWorkflow(
    name="my-cw",
    namespace=global_config.namespace,
    schedule="*/1 * * * *",
    entrypoint="main",
) as cw:

    cmd_args = ["-c", "echo 'hello world'"]

    main = Container(
        name="main",
        command=["/bin/sh"],
        args=cmd_args,
    )

    pass

cw.create()

time.sleep(10)

with CronWorkflow(
    name="my-cw",
    namespace=global_config.namespace,
    schedule="2 * * * *", # new schedule
    entrypoint="main",
) as ncw:

    cmd_args = ["-c", "echo 'hello world'"]

    main = Container(
        name="main",
        command=["/bin/sh"],
        args=cmd_args,
    )

ncw.update()

```
</details>

Signed-off-by: iameskild <eskild.eriksen122@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: CronWorkflow.update function
2 participants