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

Added the ability to change a server capability name #5605

Merged
merged 17 commits into from
Mar 10, 2021

Conversation

rimashah25
Copy link
Contributor

@rimashah25 rimashah25 commented Mar 4, 2021

What does this PR (Pull Request) do?

Which Traffic Control components are affected by this PR?

  • Documentation
  • Traffic Ops
  • Traffic Portal

What is the best way to verify this PR?

Method 1: curl PUT API end point `api/4.0/server_capabilities?name=<server_capability_name>

{"alerts":[{
    "text":"server capability was updated.",
    "level":"success"}],
  "response":{"RequestedName":"tar","name":"tar","lastUpdated":"2021-03-04 02:50:49+00"}
}

Method 2: Using TP's page for server capability to edit(update) the name and verify name was updated with right message

Method 3: Run TP's end to end test.

Method 4: Run TO's API-v4 test casse

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

The following criteria are ALL met by this PR

  • This PR includes tests
  • This PR includes documentation
  • This PR includes an update to CHANGELOG.md
  • This PR includes any and all required license headers
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

Additional Information

@rimashah25 rimashah25 marked this pull request as ready for review March 4, 2021 22:14
@zrhoffman zrhoffman added improvement The functionality exists but it could be improved in some way. Traffic Ops related to Traffic Ops Traffic Portal v1 related to Traffic Portal version 1 labels Mar 4, 2021
Copy link
Member

@mitchell852 mitchell852 left a comment

Choose a reason for hiding this comment

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

TP code looks good!

@mitchell852
Copy link
Member

I don't understand the RequestedName property of the PUT response. Is this something new we are doing?

@rimashah25
Copy link
Contributor Author

I don't understand the RequestedName property of the PUT response. Is this something new we are doing?

Yes, so server_capability API uses CRUDer function. In CRUD Update, there is a function (SetKeys) that basically sets the name field (primary key) to that of the param key value. Due to which, we lose the new name (RequestedName) that the user is trying to change to. To ensure, we don't lose the new name and at the same time ensure CRUDer functionality says un-impacted, this new variable was introduced.

@ocket8888
Copy link
Contributor

Yes, so server_capability API uses CRUDer function. In CRUD Update, there is a function (SetKeys) that basically sets the name field (primary key) to that of the param key value. Due to which, we lose the new name (RequestedName) that the user is trying to change to. To ensure, we don't lose the new name and at the same time ensure CRUDer functionality says un-impacted, this new variable was introduced.

Not that I think you need to/should do so in this PR, but this is something that would motivate me to rewrite the endpoint to not use the "CRUDer"

@rimashah25
Copy link
Contributor Author

rimashah25 commented Mar 10, 2021

Yes, so server_capability API uses CRUDer function. In CRUD Update, there is a function (SetKeys) that basically sets the name field (primary key) to that of the param key value. Due to which, we lose the new name (RequestedName) that the user is trying to change to. To ensure, we don't lose the new name and at the same time ensure CRUDer functionality says un-impacted, this new variable was introduced.

Not that I think you need to/should do so in this PR, but this is something that would motivate me to rewrite the endpoint to not use the "CRUDer"

IMO, we should get rid of all CRUDer functionality. This PR is a good example of how we end up applying a bandaid to not impact existing functionality

Copy link
Contributor

@srijeet0406 srijeet0406 left a comment

Choose a reason for hiding this comment

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

Code looks good. API/ unit tests passed. Verified manually that I could change the server capability name from the UI and the API. LGTM.

@ocket8888
Copy link
Contributor

IMO, we should get rid of all CRUDer functionality. This PR is a good example of how we end up applying a bandaid to not impact exists functionality

I would love that. It's just hard to justify the time it would take to yank all of that stuff out.

@zrhoffman zrhoffman merged commit 257af6a into apache:master Mar 10, 2021
@rimashah25 rimashah25 deleted the improvement/git-5479 branch March 10, 2021 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement The functionality exists but it could be improved in some way. Traffic Ops related to Traffic Ops Traffic Portal v1 related to Traffic Portal version 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the ability to change a server capability name
5 participants