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

Rename 'delete_application' to 'unregister_application' in clipper_admin #653

Merged
merged 3 commits into from May 12, 2019

Conversation

Projects
None yet
4 participants
@withsmilo
Copy link
Collaborator

commented Mar 22, 2019

  • related issue : #603

@withsmilo withsmilo self-assigned this Mar 22, 2019

@AmplabJenkins

This comment has been minimized.

Copy link

commented Mar 22, 2019

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/1814/
Test PASSed.

@withsmilo

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 25, 2019

Jenkins ok to test

@AmplabJenkins

This comment has been minimized.

Copy link

commented Mar 25, 2019

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/1820/
Test PASSed.

@withsmilo withsmilo requested review from simon-mo and RehanSD Mar 25, 2019

@simon-mo

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

@withsmilo can you still create a new method named delete_application and call unregister_application underneath. So existing code doesn’t need to face this breaking change.

In delete application, we can even throw a warning saying this api is deprecated and is aliasing unregistered application

@withsmilo

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 31, 2019

@simon-mo , I understand it. It's great idea! I will refactor this PR as soon as possible.

@rkooo567
Copy link
Collaborator

left a comment

For me, it is good to go if merge conflict is resolved

@withsmilo

This comment has been minimized.

Copy link
Collaborator Author

commented May 12, 2019

@rkooo567 : I resolved the merge conflict, and added an warning message to the old API(delete_application) reflecting @simon-mo 's opnion.

@AmplabJenkins

This comment has been minimized.

Copy link

commented May 12, 2019

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/1947/
Test FAILed.

@AmplabJenkins

This comment has been minimized.

Copy link

commented May 12, 2019

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/1948/
Test FAILed.

@withsmilo

This comment has been minimized.

Copy link
Collaborator Author

commented May 12, 2019

Jenkins test this please

@AmplabJenkins

This comment has been minimized.

Copy link

commented May 12, 2019

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Clipper-PRB/1949/
Test PASSed.

@rkooo567
Copy link
Collaborator

left a comment

I think it is good to go. I will just approve because this PR is simple enough. @simon-mo

@withsmilo

This comment has been minimized.

Copy link
Collaborator Author

commented May 12, 2019

@rkooo567 : Thanks! I will merge this if @simon-mo confirms

@withsmilo withsmilo merged commit 8051a4e into ucbrise:develop May 12, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
default Merged build finished.
Details

@withsmilo withsmilo deleted the withsmilo:rename_api branch May 12, 2019

@withsmilo

This comment has been minimized.

Copy link
Collaborator Author

commented May 12, 2019

@simon-mo Thanks! 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.