You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2022/01/07 15:30:42 UTC
[GitHub] [trafficcontrol] ocket8888 opened a new pull request #6453: Add the ability to edit and delete existing Jobs to TPv2
ocket8888 opened a new pull request #6453:
URL: https://github.com/apache/trafficcontrol/pull/6453
Adds to TPv2 the ability to delete and edit existing jobs (jobs can only be edited if they haven't started, only deleted if they either haven't started or have finished).
<hr/>
## Which Traffic Control components are affected by this PR?
- Traffic Portal (experimental v2)
## What is the best way to verify this PR?
Make sure the tests pass, make a Job that will run in the future, edit it, delete it, make a Job that's currently running (only possible though database manipulation or waiting for long periods of time), make sure you can't edit or delete it, make a Job that's finished running (only possible through database manipulation or waiting for long periods of time) and make sure you can't edit it, then delete it.
## PR submission checklist
- [x] This PR has tests
- [ ] This PR has documentation
- [ ] This PR has a CHANGELOG.md entry
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6453: Add the ability to edit and delete existing Jobs to TPv2
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6453:
URL: https://github.com/apache/trafficcontrol/pull/6453#discussion_r782604030
##########
File path: experimental/traffic-portal/src/app/core/invalidation-jobs/invalidation-jobs.component.html
##########
@@ -13,6 +13,30 @@
-->
<tp-header title="{{deliveryservice ? deliveryservice.displayName : 'Loading'}} - Content Invalidation Jobs"></tp-header>
<ul>
- <li *ngFor="let j of jobs" [hidden]="endDate(j) < now"><code>{{j.assetUrl}}</code> (active from <time [dateTime]="j.startTime">{{j.startTime | date:'medium'}}</time> to <time [dateTime]="endDate(j)">{{endDate(j) | date:'medium'}})</time></li>
+ <li *ngFor="let j of jobs">
+ <code>{{j.assetUrl}}</code> (active from <time [dateTime]="j.startTime">{{j.startTime | date:'medium'}}</time> to <time [dateTime]="endDate(j)">{{endDate(j) | date:'medium'}})</time>
+ <button
+ mat-icon-button
+ type="button"
+ color="accent"
+ title="edit this content invalidation job"
+ aria-label="edit this content invalidation job"
Review comment:
I'll just change them both for consistency
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus merged pull request #6453: Add the ability to edit and delete existing Jobs to TPv2
Posted by GitBox <gi...@apache.org>.
shamrickus merged pull request #6453:
URL: https://github.com/apache/trafficcontrol/pull/6453
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on a change in pull request #6453: Add the ability to edit and delete existing Jobs to TPv2
Posted by GitBox <gi...@apache.org>.
shamrickus commented on a change in pull request #6453:
URL: https://github.com/apache/trafficcontrol/pull/6453#discussion_r782558838
##########
File path: experimental/traffic-portal/src/app/core/invalidation-jobs/new-invalidation-job-dialog/new-invalidation-job-dialog.component.ts
##########
@@ -45,15 +89,29 @@ export class NewInvalidationJobDialogComponent {
/** A subscribable that tracks whether the new job's regexp is valid. */
Review comment:
Because I like round numbers, 178 just seemed arbitrary but if there's a reason behind it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on a change in pull request #6453: Add the ability to edit and delete existing Jobs to TPv2
Posted by GitBox <gi...@apache.org>.
shamrickus commented on a change in pull request #6453:
URL: https://github.com/apache/trafficcontrol/pull/6453#discussion_r782377799
##########
File path: experimental/traffic-portal/src/app/core/invalidation-jobs/invalidation-jobs.component.html
##########
@@ -13,6 +13,30 @@
-->
<tp-header title="{{deliveryservice ? deliveryservice.displayName : 'Loading'}} - Content Invalidation Jobs"></tp-header>
<ul>
- <li *ngFor="let j of jobs" [hidden]="endDate(j) < now"><code>{{j.assetUrl}}</code> (active from <time [dateTime]="j.startTime">{{j.startTime | date:'medium'}}</time> to <time [dateTime]="endDate(j)">{{endDate(j) | date:'medium'}})</time></li>
+ <li *ngFor="let j of jobs">
+ <code>{{j.assetUrl}}</code> (active from <time [dateTime]="j.startTime">{{j.startTime | date:'medium'}}</time> to <time [dateTime]="endDate(j)">{{endDate(j) | date:'medium'}})</time>
+ <button
+ mat-icon-button
+ type="button"
+ color="accent"
+ title="edit this content invalidation job"
+ aria-label="edit this content invalidation job"
Review comment:
First letter should be capitalized here and in title?
##########
File path: experimental/traffic-portal/src/app/core/invalidation-jobs/new-invalidation-job-dialog/new-invalidation-job-dialog.component.ts
##########
@@ -45,15 +89,29 @@ export class NewInvalidationJobDialogComponent {
/** A subscribable that tracks whether the new job's regexp is valid. */
Review comment:
Not related to your changes but shouldn't the default TTL be 180?
##########
File path: experimental/traffic-portal/src/app/core/invalidation-jobs/invalidation-jobs.component.html
##########
@@ -13,6 +13,30 @@
-->
<tp-header title="{{deliveryservice ? deliveryservice.displayName : 'Loading'}} - Content Invalidation Jobs"></tp-header>
<ul>
- <li *ngFor="let j of jobs" [hidden]="endDate(j) < now"><code>{{j.assetUrl}}</code> (active from <time [dateTime]="j.startTime">{{j.startTime | date:'medium'}}</time> to <time [dateTime]="endDate(j)">{{endDate(j) | date:'medium'}})</time></li>
+ <li *ngFor="let j of jobs">
+ <code>{{j.assetUrl}}</code> (active from <time [dateTime]="j.startTime">{{j.startTime | date:'medium'}}</time> to <time [dateTime]="endDate(j)">{{endDate(j) | date:'medium'}})</time>
+ <button
+ mat-icon-button
+ type="button"
+ color="accent"
+ title="edit this content invalidation job"
+ aria-label="edit this content invalidation job"
+ [disabled]="j.startTime <= now"
+ (click)="editJob(j)"
+ >
+ <fa-icon [icon]="editIcon"></fa-icon>
+ </button>
+ <button
+ mat-icon-button
+ type="button"
+ color="warn"
+ title="delete this content invalidation job"
+ aria-label="delete this content invalidation job"
Review comment:
Same as above
##########
File path: experimental/traffic-portal/src/app/core/invalidation-jobs/new-invalidation-job-dialog/new-invalidation-job-dialog.component.ts
##########
@@ -45,15 +89,29 @@ export class NewInvalidationJobDialogComponent {
/** A subscribable that tracks whether the new job's regexp is valid. */
public readonly regexpIsValid = new Subject<string>();
+ private readonly job: InvalidationJob | undefined;
+ private readonly dsID: number;
+
constructor(
private readonly dialogRef: MatDialogRef<NewInvalidationJobDialogComponent>,
private readonly jobAPI: InvalidationJobService,
- @Inject(MAT_DIALOG_DATA) private readonly dsID: number
+ @Inject(MAT_DIALOG_DATA) data: DialogData
) {
+ this.job = data.job;
+ if (this.job) {
+ this.startDate = this.job.startTime;
+ const startTime = timeStringFromDate(this.job.startTime);
+ this.startMinTime = startTime;
+ this.startTime.setValue(startTime);
Review comment:
`this.startDate`, `this.startMinTime`, and ` this.startTime` all all overwritten after this if block causing the values to be based on current time instead of the existing job
##########
File path: experimental/traffic-portal/src/app/core/invalidation-jobs/new-invalidation-job-dialog/new-invalidation-job-dialog.component.ts
##########
@@ -45,15 +89,29 @@ export class NewInvalidationJobDialogComponent {
/** A subscribable that tracks whether the new job's regexp is valid. */
public readonly regexpIsValid = new Subject<string>();
+ private readonly job: InvalidationJob | undefined;
+ private readonly dsID: number;
+
constructor(
private readonly dialogRef: MatDialogRef<NewInvalidationJobDialogComponent>,
private readonly jobAPI: InvalidationJobService,
- @Inject(MAT_DIALOG_DATA) private readonly dsID: number
+ @Inject(MAT_DIALOG_DATA) data: DialogData
) {
+ this.job = data.job;
+ if (this.job) {
+ this.startDate = this.job.startTime;
+ const startTime = timeStringFromDate(this.job.startTime);
+ this.startMinTime = startTime;
+ this.startTime.setValue(startTime);
+ this.ttl.setValue(parseInt(this.job.parameters.split(":")[1], 10));
+ const regexp = this.job.assetUrl.split("/").slice(3).join("/") || "/";
+ console.log("setting regexp value from", this.job.assetUrl, "to", regexp);
Review comment:
Unnecessary log
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6453: Add the ability to edit and delete existing Jobs to TPv2
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6453:
URL: https://github.com/apache/trafficcontrol/pull/6453#discussion_r782562266
##########
File path: experimental/traffic-portal/src/app/core/invalidation-jobs/invalidation-jobs.component.html
##########
@@ -13,6 +13,30 @@
-->
<tp-header title="{{deliveryservice ? deliveryservice.displayName : 'Loading'}} - Content Invalidation Jobs"></tp-header>
<ul>
- <li *ngFor="let j of jobs" [hidden]="endDate(j) < now"><code>{{j.assetUrl}}</code> (active from <time [dateTime]="j.startTime">{{j.startTime | date:'medium'}}</time> to <time [dateTime]="endDate(j)">{{endDate(j) | date:'medium'}})</time></li>
+ <li *ngFor="let j of jobs">
+ <code>{{j.assetUrl}}</code> (active from <time [dateTime]="j.startTime">{{j.startTime | date:'medium'}}</time> to <time [dateTime]="endDate(j)">{{endDate(j) | date:'medium'}})</time>
+ <button
+ mat-icon-button
+ type="button"
+ color="accent"
+ title="edit this content invalidation job"
+ aria-label="edit this content invalidation job"
Review comment:
I don't think so - I don't object, it's just that there's no requirement I can see anywhere in WHATWG or MDN that says they should be capitalized and/or be complete sentences.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #6453: Add the ability to edit and delete existing Jobs to TPv2
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #6453:
URL: https://github.com/apache/trafficcontrol/pull/6453#discussion_r782556808
##########
File path: experimental/traffic-portal/src/app/core/invalidation-jobs/new-invalidation-job-dialog/new-invalidation-job-dialog.component.ts
##########
@@ -45,15 +89,29 @@ export class NewInvalidationJobDialogComponent {
/** A subscribable that tracks whether the new job's regexp is valid. */
Review comment:
Why 180? I just picked 178 because it was the median value used in a production environment I surveyed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] shamrickus commented on a change in pull request #6453: Add the ability to edit and delete existing Jobs to TPv2
Posted by GitBox <gi...@apache.org>.
shamrickus commented on a change in pull request #6453:
URL: https://github.com/apache/trafficcontrol/pull/6453#discussion_r782567138
##########
File path: experimental/traffic-portal/src/app/core/invalidation-jobs/invalidation-jobs.component.html
##########
@@ -13,6 +13,30 @@
-->
<tp-header title="{{deliveryservice ? deliveryservice.displayName : 'Loading'}} - Content Invalidation Jobs"></tp-header>
<ul>
- <li *ngFor="let j of jobs" [hidden]="endDate(j) < now"><code>{{j.assetUrl}}</code> (active from <time [dateTime]="j.startTime">{{j.startTime | date:'medium'}}</time> to <time [dateTime]="endDate(j)">{{endDate(j) | date:'medium'}})</time></li>
+ <li *ngFor="let j of jobs">
+ <code>{{j.assetUrl}}</code> (active from <time [dateTime]="j.startTime">{{j.startTime | date:'medium'}}</time> to <time [dateTime]="endDate(j)">{{endDate(j) | date:'medium'}})</time>
+ <button
+ mat-icon-button
+ type="button"
+ color="accent"
+ title="edit this content invalidation job"
+ aria-label="edit this content invalidation job"
Review comment:
I suppose it really doesn't matter for `aria-labels` but for the user I think it makes more sense if the title to be.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org