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.

Sign up for GitHub

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

Jump to bottom

Add comment field to parameters #7845

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Add comment field to parameters #7845

wants to merge 10 commits into from

Conversation

ntheanh201
Copy link
Contributor

@ntheanh201 ntheanh201 commented Oct 23, 2023 •
edited
Loading

Closes: #7456

Add field "comment" to parameter

Which Traffic Control components are affected by this PR?

  • Traffic Ops
  • Traffic Portal

What is the best way to verify this PR?

  • Enable show parameters "comment" field in traffic_portal_properties.json: properties.parameters.comment.show = true
  • Provisioning CDN in a Box, select Configure/Parameters, 1 more columns "comment" should show
  • Create/Update Parameters with "comment"

If this is a bugfix, which Traffic Control versions contained the bug?

PR submission checklist

@ntheanh201 ntheanh201 changed the title Add comments field Add comment field to parameters Oct 23, 2023
@codecov
Copy link

codecov bot commented Oct 25, 2023 •
edited
Loading

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (fa231d7) 65.64% compared to head (c6c3fd7) 31.83%.
Report is 33 commits behind head on master.

Files Patch % Lines
...fic_ops/traffic_ops_golang/parameter/parameters.go 14.28% 18 Missing ⚠️
Additional details and impacted files 
@@              Coverage Diff              @@
##             master    #7845       +/-   ##
=============================================
- Coverage     65.64%   31.83%   -33.81%     
  Complexity       98       98               
=============================================
  Files           323      719      +396     
  Lines         12836    82740    +69904     
  Branches        970      970               
=============================================
+ Hits           8426    26340    +17914     
- Misses         4050    54241    +50191     
- Partials        360     2159     +1799
Flag Coverage Δ
golib_unit 53.85% <ø> (?)
grove_unit 12.02% <ø> (?)
t3c_unit 5.88% <ø> (?)
traffic_monitor_unit 25.47% <ø> (?)
traffic_ops_integration 69.42% <ø> (ø)
traffic_ops_unit 21.67% <14.28%> (?)
traffic_portal_v2 74.35% <ø> (ø)
traffic_stats_unit 10.78% <ø> (?)
unit_tests 29.15% <14.28%> (-45.21%) ⬇️
v3 57.79% <ø> (ø)
v4 79.18% <ø> (ø)
v5 78.58% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ocket8888
Copy link
Contributor

I am -1 on this idea. The promise of a comments field being used only for humans' benefits has been repeatedly broken on other objects, and a Parameter ought to be too small to express anything complex enough to require a comment. In fact, Parameters as objects should not even exist in their own right, IMO, and - especially now that Profile layering is possible - instead just be properties of Profiles. In some respects, they are represented that way already, and that means that this amounts to, in those cases, having an unbounded number of free-form text fields on a collection of free-form text fields on a Profile.

@ntheanh201
Copy link
Contributor Author

at way already, and that means that this amounts to, in those cases, having an unbounded number of free-form text fields on a collection of free-form text fields on a Profile.

IMO I still need a description of some special parameters that are required for the Profile

@ocket8888
Copy link
Contributor

Any Parameter that is required for a Profile of some given type ought not to be optionally provided by the data model; instead it should be a required property of a Profile.

@mitchell852
Copy link
Member

mitchell852 commented Nov 29, 2023 •
edited
Loading

@ntheanh201 - can you make it optional to show this comment field in traffic portal? for example, add a config setting like parameter.comment.show = false to the traffic portal properties file and key off that in the UI.

@ntheanh201
Copy link
Contributor Author

@ntheanh201 - can you make it optional to show this comment field in traffic portal? for example, add a config setting like parameter.comment.show = false to the traffic portal properties file and key off that in the UI.

Updated like this, please take a look!

@ocket8888
Copy link
Contributor

I'm still -1 on this idea. Adding tech debt doesn't seem like a good way to resolve tech debt.

kdamichie
kdamichie suggested changes Dec 18, 2023
View reviewed changes
traffic_ops/app/db/create_tables.sql Outdated Show resolved Hide resolved
kdamichie
kdamichie approved these changes Dec 20, 2023
View reviewed changes
Copy link
Contributor

@kdamichie kdamichie left a comment

Choose a reason for hiding this comment

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

Looks good. Comment field is present under parameters

kdamichie
kdamichie suggested changes Jan 3, 2024
View reviewed changes
Copy link
Contributor

@kdamichie kdamichie left a comment

Choose a reason for hiding this comment

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

Sorry to amend my response. But can you verify this test failure. Looks like it needs a test update.

@kdamichie
Copy link
Contributor

@ntheanh201 ^

@ntheanh201
Copy link
Contributor Author

Sorry to amend my response. But can you verify this test failure. Looks like it needs a test update.

It has not failed because of this code, this failure is related to the integration tests themselves being broken, you can see some other commits

kdamichie
kdamichie approved these changes Jan 4, 2024
View reviewed changes
Copy link
Contributor

@kdamichie kdamichie left a comment

Choose a reason for hiding this comment

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

Approved

@ntheanh201 ntheanh201 mentioned this pull request Jan 25, 2024
Open
@zrhoffman zrhoffman added Traffic Ops related to Traffic Ops Traffic Portal v1 related to Traffic Portal version 1 low impact affects only a small portion of a CDN, and cannot itself break one improvement The functionality exists but it could be improved in some way. medium difficulty the estimated level of effort to resolve this issue is medium labels Feb 1, 2024
@zrhoffman
Copy link
Member

@ocket8888 Responded in #7456 (comment) because a PR is not the place to discuss the validity of an Issue. With no response after a week, I'll assume you agree and will merge

@ntheanh201
Copy link
Contributor Author

@zrhoffman can you merge?

@zrhoffman
Copy link
Member

@zrhoffman can you merge?

If we can verify that no committers are against #7845 being merged, yes. I'll clarify in Issue #7456 :)

zrhoffman
zrhoffman requested changes Oct 18, 2024
View reviewed changes
Copy link
Member

@zrhoffman zrhoffman left a comment

Choose a reason for hiding this comment

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

Just a small change needed with the migration timestamp, then it should be good to merge. Thanks!

traffic_ops/app/db/migrations/2023121910512261_add_comment_parameter.down.sql
@@ -0,0 +1,18 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

The timestamp on this migration, 2023121910512261, is 10 months old. Would you please give it a more recent timestamp (for example, 2024101800000000)?

traffic_ops/app/db/migrations/2023121910512261_add_comment_parameter.up.sql
@@ -0,0 +1,18 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here, the timestamp 2023121910512261 is from last year and should be replaced with something more recent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zrhoffman zrhoffman zrhoffman requested changes

@kdamichie kdamichie kdamichie approved these changes

Assignees
No one assigned
Labels
improvement The functionality exists but it could be improved in some way. low impact affects only a small portion of a CDN, and cannot itself break one medium difficulty the estimated level of effort to resolve this issue is medium Traffic Ops related to Traffic Ops Traffic Portal v1 related to Traffic Portal version 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add notes/comments field to parameters
5 participants
@ntheanh201 @ocket8888 @mitchell852 @kdamichie @zrhoffman