Skip to content

Remove Support of hidden_envoy_deprecated_UNSUPPORTED_REST_LEGACY#17972

Closed
ankatare wants to merge 2 commits intoenvoyproxy:mainfrom
ankatare:hidden_envoy_deprecated_UNSUPPORTED_REST_LEGACY
Closed

Remove Support of hidden_envoy_deprecated_UNSUPPORTED_REST_LEGACY#17972
ankatare wants to merge 2 commits intoenvoyproxy:mainfrom
ankatare:hidden_envoy_deprecated_UNSUPPORTED_REST_LEGACY

Conversation

@ankatare
Copy link
Copy Markdown
Contributor

@ankatare ankatare commented Sep 3, 2021

Signed-off-by: Abhay Narayan Katare abhay.katare@india.nec.com

Commit Message: Remove Support of hidden_envoy_deprecated_UNSUPPORTED_REST_LEGACY
Additional Description: Another PR of type remove_v2_support. first PR is #16274
Risk Level: LOW
Testing: bazel test on test/common/config/..
Docs Changes: NA
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Abhay Narayan Katare <abhay.katare@india.nec.com>
@ankatare
Copy link
Copy Markdown
Contributor Author

ankatare commented Sep 3, 2021

@tyxia PTAL
cc @htuch

@tyxia
Copy link
Copy Markdown
Member

tyxia commented Sep 3, 2021

@tyxia PTAL
cc @htuch

Thanks for working on this!

@htuch and I have discussed about this before and we have decided to keep this code. We want to keep this exception to force graceful rejection to any invalid configuration.

Later when we get rid of generated_api_version we can then either use a PGV annotation or the DEPRECATED_AND_UNAVAILABLE_DO_NOT_USE name to perform such rejection.

So this PR can be closed. Please let me know if you have any questions. Thanks!

@ankatare
Copy link
Copy Markdown
Contributor Author

ankatare commented Sep 3, 2021

@tyxia Ok and thanks for quick review. Just waiting signal from @htuch to decide.

@mattklein123
Copy link
Copy Markdown
Member

Instead of just closing the PR, can you revert the code change and add a comment so that someone doesn't come along and try deleting it again?

/wait

@tyxia
Copy link
Copy Markdown
Member

tyxia commented Sep 3, 2021

Instead of just closing the PR, can you revert the code change and add a comment so that someone doesn't come along and try deleting it again?

/wait

I agree. Good idea!

@tyxia
Copy link
Copy Markdown
Member

tyxia commented Sep 3, 2021

@tyxia Ok and thanks for quick review. Just waiting signal from @htuch to decide.

Hi @ankatare, I think we don't need to wait for the signal from @htuch as he already made the call in the other PR here #17804

As @mattklein123 suggested, Could you go ahead to add the comment to explain why we want to keep it? The comment could be something like Keep this exception to force graceful rejection to any invalid configuration. Later when we get rid of generated_api_version, we can then either use a PGV annotation or the DEPRECATED_AND_UNAVAILABLE_DO_NOT_USE name to perform such rejection.

Thanks!

@ankatare
Copy link
Copy Markdown
Contributor Author

ankatare commented Sep 6, 2021

Instead of just closing the PR, can you revert the code change and add a comment so that someone doesn't come along and try deleting it again?

/wait

@mattklein123 Sure. Will do it ASAP

@ankatare
Copy link
Copy Markdown
Contributor Author

ankatare commented Sep 6, 2021

@tyxia Ok and thanks for quick review. Just waiting signal from @htuch to decide.

Hi @ankatare, I think we don't need to wait for the signal from @htuch as he already made the call in the other PR here #17804

As @mattklein123 suggested, Could you go ahead to add the comment to explain why we want to keep it? The comment could be something like Keep this exception to force graceful rejection to any invalid configuration. Later when we get rid of generated_api_version, we can then either use a PGV annotation or the DEPRECATED_AND_UNAVAILABLE_DO_NOT_USE name to perform such rejection.

Thanks!

@tyxia Sure and Thanks for suggetion

@ankatare ankatare force-pushed the hidden_envoy_deprecated_UNSUPPORTED_REST_LEGACY branch from 5fec273 to e59db5b Compare September 7, 2021 09:05
@ankatare
Copy link
Copy Markdown
Contributor Author

ankatare commented Sep 7, 2021

/wait

@ankatare
Copy link
Copy Markdown
Contributor Author

ankatare commented Sep 7, 2021

seems some local git issues. please DO NOT review it. i will push changes again ASAP.

@ankatare
Copy link
Copy Markdown
Contributor Author

ankatare commented Sep 7, 2021

@tyxia i need your help to revert changes as suggest by @mattklein123 . please ping me on slack as soon you are available

@tyxia
Copy link
Copy Markdown
Member

tyxia commented Sep 7, 2021

@tyxia i need your help to revert changes as suggest by @mattklein123 . please ping me on slack as soon you are available

I don't see your message in the slack...
What is your issue with revert? In general, for staged files you can git reset HEAD <file>…​ to unstage. I guess your issue is with the change has been pushed, you can git checkout HEAD^ -- /path/to/file to previous version OR git checkout [commit-ref] -- /path/to/file to specified version.

Take a look at links below and hope they help:
https://git-scm.com/book/en/v2/Git-Basics-Undoing-Things
https://stackoverflow.com/questions/18357511/git-remove-committed-file-after-push/18357621
https://stackoverflow.com/questions/215718/how-can-i-reset-or-revert-a-file-to-a-specific-revision

@ankatare
Copy link
Copy Markdown
Contributor Author

ankatare commented Sep 9, 2021

@tyxia thanks for your comments and help .. i am not well from couple of days :( . I will do it tomorrow !!!

@ankatare
Copy link
Copy Markdown
Contributor Author

@tyxia I am really sorry for delay. just wondering i can see commit e59db5baae506ad6de16dabd23ab24079b4cfaa3 with same commit message, have you done this already ?

@htuch
Copy link
Copy Markdown
Member

htuch commented Sep 15, 2021

Can you merge with #18091?

@tyxia
Copy link
Copy Markdown
Member

tyxia commented Sep 15, 2021

@tyxia I am really sorry for delay. just wondering i can see commit e59db5b with same commit message, have you done this already ?

I haven't done this yet but I am happy to do this PR if you run into any issues/you are OK. Just let me know :)

@ankatare
Copy link
Copy Markdown
Contributor Author

@tyxia Thanks. Let me check this now. I will ping you to take further if found any issues.

…uration. Later when we get rid of generated_api_version, we can then either use a PGV annotation or the DEPRECATED_AND_UNAVAILABLE_DO_NOT_USE name to perform such rejection.

Signed-off-by: Abhay Narayan Katare <abhay.katare@india.nec.com>
@ankatare ankatare force-pushed the hidden_envoy_deprecated_UNSUPPORTED_REST_LEGACY branch from e59db5b to 0fe8756 Compare September 23, 2021 05:31
@ankatare
Copy link
Copy Markdown
Contributor Author

seems this is already incorporated by @htuch in commit b0e15260326d0d6175bc2271fb712f2f1efb029e. so we can close this issue now.

@ankatare ankatare closed this Sep 23, 2021
@ankatare
Copy link
Copy Markdown
Contributor Author

closing this PR as code changes done already.

@ankatare ankatare deleted the hidden_envoy_deprecated_UNSUPPORTED_REST_LEGACY branch September 29, 2021 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants