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

frontend: CronJob details refactor #2745

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

sniok
Copy link
Contributor

@sniok sniok commented Jan 13, 2025

Main point of this refactor is to use patch function when using suspend/resume button. This will only send a { spec: { suspend: true/false } } update to the server instead of sending the whole object. Which will make it simpler and avoid any resourceVersion conflitcs.

How to test:

  • Open any CronJob
  • Click on Suspend/Resume
  • Click on Spawn Job
  • Edit the CronJob

There shouldn't be any errors and should behave as expected

Sample CronJob yaml
apiVersion: batch/v1
kind: CronJob
metadata:
  name: yearly-cronjob
  namespace: default
spec:
  schedule: "@yearly"
  jobTemplate:
    spec:
      template:
        spec:
          containers:
          - name: yearly-job
            image: busybox:latest
            command:
            - /bin/sh
            - -c
            - "echo Happy New Year!; date"
          restartPolicy: OnFailure

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jan 13, 2025
@sniok sniok force-pushed the cronjob-details-refactor branch from a24b1fd to 90ebf2b Compare January 13, 2025 14:32
@sniok sniok requested a review from a team January 13, 2025 14:48
Copy link
Collaborator

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

This branch is actually introducing issues now. main works fine now with the recent fixes, but with this branch I notice:

  • The suspend/resume button is not disabled when the request is being sent;
  • The suspend/resume button and the suspend state in the table doesn't reflect the actual state of the resource

So it seems like the resource is not getting updated.

Previously it was typed as if it accepted Patch Operations but in fact
it accepts json merge patch. This method wasn't used anywhere so this
bug wasn't visible

Signed-off-by: Oleksandr Dubenko <[email protected]>
@sniok sniok force-pushed the cronjob-details-refactor branch from 90ebf2b to 559e7c5 Compare January 14, 2025 09:44
@sniok
Copy link
Contributor Author

sniok commented Jan 14, 2025

Fixed disabling suspend/resume button.
The state in the table now works, just needed a rebase on main

Copy link
Collaborator

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

Approved but there's something I think we should memoize.

frontend/src/components/cronjob/Details.tsx Outdated Show resolved Hide resolved
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jan 14, 2025
@sniok sniok force-pushed the cronjob-details-refactor branch from 559e7c5 to 2c51759 Compare January 14, 2025 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants