Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Add deprecated message to cachegroup/:parameter_id/parameter resp#3908

Closed
mhoppa wants to merge 1 commit into
apache:masterfrom
mhoppa:feature/depreciate_cg_parameter_path
Closed

Add deprecated message to cachegroup/:parameter_id/parameter resp#3908
mhoppa wants to merge 1 commit into
apache:masterfrom
mhoppa:feature/depreciate_cg_parameter_path

Conversation

@mhoppa

@mhoppa mhoppa commented Sep 6, 2019

Copy link
Copy Markdown
Contributor

What does this PR (Pull Request) do?

  • This PR is not related to any Issue

Which Traffic Control components are affected by this PR?

  • Traffic Ops

There is not a change to documentation as it is already mark deprecated in there.

What is the best way to verify this PR?

Start up Perl TO locally with change and make a request to it. On success you should receive back a response that has an alert section with deprecate messaging

If this is a bug fix, what versions of Traffic Control are affected?

The following criteria are ALL met by this PR

  • This PR does not includes tests
  • I have explained why documentation is unnecessary
  • This PR does include an update to CHANGELOG.md as such an update is not necessary
  • This PR includes any and all required license headers
  • This PR does not include a database migration
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

@ocket8888

Copy link
Copy Markdown
Contributor

I believe deprecations normally have to go through the mailing list. You probably wanna ask about Cache Group Parameters in general.

@mhoppa

mhoppa commented Sep 6, 2019

Copy link
Copy Markdown
Contributor Author

@ocket8888 I noticed it was already marked as such in the documentation https://traffic-control-cdn.readthedocs.io/en/latest/api/cachegroup_parameterID_parameter.html

Does that mean it has already gone through mailing list?

@asf-ci

asf-ci commented Sep 6, 2019

Copy link
Copy Markdown
Contributor

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4213/
Test PASSed.

@rawlinp

rawlinp commented Sep 6, 2019

Copy link
Copy Markdown
Contributor

Cache Group Params in general probably can't be deprecated since I'm pretty sure we actually use and depend on them (despite the absurdity of associating parameters to cachegroups).

About the mailing list, we might want to group up all the endpoints we think we should deprecate in 1.1 and discuss the entire set at once on the mailing list. It will probably get pretty messy to discuss the deprecation of API endpoints one at a time on the mailing list. We might get better feedback with a full list too, because users can just take the whole list and check their usage of all the deprecated endpoints at once.

@ocket8888

ocket8888 commented Sep 6, 2019

Copy link
Copy Markdown
Contributor

RE: the docs deprecation stuff: I did that and I shouldn't have. When I was writing that endpoint's docs I'm sure I asked in an internal Slack channel "what is this?" and was told "we don't use that anymore" or something, so I added the deprecation notice without going through the proper procedure.

I like @rawlinp's idea about a bulk deprecation thread. Seems it'll save a lot of time.

@mitchell852 mitchell852 added the Traffic Ops related to Traffic Ops label Sep 9, 2019
}

$self->success( { cachegroups => \@data } );
$self->success( { cachegroups => \@data }, "API path is deprecated use /cachegroupparameters instead");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"Success" is a bit of a weird type of message for something that's more of a warning about a deprecation. This should use the "warning" level instead, imo.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what are the alert "levels"? success, error, warning and info?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i thought you could return an array of alerts. if so, what about 1 success message (for the successful assignment of a param to a cg) and 1 warning message with the deprecation message? or is that weird?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No I think that is what we want to do! THat would match the work that was done #3954

@mitchell852 mitchell852 added the tech debt rework due to choosing easy/limited solution label Oct 24, 2019
@mhoppa

mhoppa commented Nov 20, 2019

Copy link
Copy Markdown
Contributor Author

Closing pull request as how we are approaching deprecation is being decided in the mailing list and even if we did decide to deprecate in perl this PR would need some work

@mhoppa mhoppa closed this Nov 20, 2019
@mhoppa
mhoppa deleted the feature/depreciate_cg_parameter_path branch November 20, 2019 20:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

abandoned tech debt rework due to choosing easy/limited solution Traffic Ops related to Traffic Ops

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants