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

Release - Jobs DTO (#622) #844

Merged
merged 11 commits into from
Nov 15, 2023
Merged

Release - Jobs DTO (#622) #844

merged 11 commits into from
Nov 15, 2023

Conversation

HayenNico
Copy link

Description

Update for the DTO instances for the new jobs coming with release 4.4.

Motivation

Jobs can be very freely defined using the job configurations, so the DTOs only really need two parameters: the job type (to grab the proper definition from config) and the job parameters (filled in instance of the "template" field in the job config).
Some additional optional parameters are suggested that may apply in common use cases.

Fixes:

src/jobs/dto/create-job.dto.ts

  • fix: jobParams parameter no longer optional

Changes:

src/jobs/dto/create-job.dto.ts

  • new: optional parameters id, dependsOn, contactEmail
  • added swagger decorators

src/jobs/dto/update-job.dto.ts

  • new: parameter id is required in this case
  • added swagger decorators

Tests included/Docs Updated?

  • Included for each change/fix?
  • Passing? (Merge will not be approved unless this is checked)
  • Docs updated?
  • New packages used/requires npm install?
  • Toggle added for new features?

Junjiequan and others added 3 commits October 25, 2023 10:07
CreateJobDTO and UpdateJobDTO with API decorators etc.. Besides basic functionality already required in the controller some additional suggestions for properties.
@HayenNico HayenNico added the Release Jobs Jobs migration label Oct 30, 2023
@HayenNico HayenNico self-assigned this Oct 30, 2023
@HayenNico HayenNico changed the base branch from master to release-4-4 October 30, 2023 14:58
@HayenNico
Copy link
Author

Most tests fail since jobs.schema not yet implemented - will need resolution of #621 first, possibly others

@HayenNico HayenNico linked an issue Oct 30, 2023 that may be closed by this pull request
@bpedersen2
Copy link
Contributor

Needs rebase due to #854

There some eslint complaints that should get fixed as well.

@bpedersen2
Copy link
Contributor

Note that we don't run any checks against non-master branches and this would only go to release-4.4

I recommend to add new features always to master and fork off release branches only for bug-fixing.

Comment on lines 29 to 31
/*
New stuff from here
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove it

Copy link
Author

Choose a reason for hiding this comment

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

Done. I just left that in for review because I wasn't sure we needed some of those additional properties (yet).

Comment on lines 5 to 14
export class UpdateJobDto extends PartialType(CreateJobDto) {
@ApiProperty({
type: String,
required: true,
description:
"Id for the job to be updated.",
})
@IsString()
readonly id: string;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is correct and needed, as the update on job is not implemented.
We are going to have statusUpdate.
Check issue #622 for more info

Copy link
Author

Choose a reason for hiding this comment

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

Fixed. I've removed job-update.dto entirely, and added the status update instead.

Removed update-job.dto as it will not be implemented.
Added job-statusupdate.dto.
@nitrosx nitrosx changed the title Release 4.4 - Jobs DTO (#622) Release - Jobs DTO (#622) Nov 15, 2023
@HayenNico HayenNico merged commit bd383a3 into release-4-4 Nov 15, 2023
1 check passed
@HayenNico HayenNico deleted the Rel-4-4-Issue-622 branch November 15, 2023 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release Jobs Jobs migration
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Jobs migration: DTO
4 participants