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

Remove remaining Echo usages #4921

Merged
merged 12 commits into from
Dec 8, 2021
Merged

Remove remaining Echo usages #4921

merged 12 commits into from
Dec 8, 2021

Conversation

pgalic96
Copy link
Contributor

@pgalic96 pgalic96 commented Nov 25, 2021 •
edited by adriansmares
Loading

Summary

Closes #2627

Changes

  • Remove Echo usage from the Account App / Console
  • Remove remaining Echo middlewares

Testing

CI.

Regressions

This is tricky and touches lots of components. But if CI passes we are good to go.

Notes for Reviewers

@htdvisser main reviewer
@bafonins / @kschiffer please throw a quick eye if I accidentally broke existing behavior for the UI, albeit I expect the end to end tests to cover this
@johanstokking / @michalborkowski96 no need to review.

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
  • Documentation: Relevant documentation is added or updated.
  • Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
  • Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

@pgalic96 pgalic96 self-assigned this Nov 25, 2021
@github-actions github-actions bot added c/console This is related to the Console ui/web This is related to a web interface labels Nov 25, 2021
adriansmares
adriansmares reviewed Nov 25, 2021
View reviewed changes
pkg/console/console.go Outdated Show resolved Hide resolved
pkg/console/console.go Outdated Show resolved Hide resolved
pkg/console/console.go Outdated Show resolved Hide resolved
pkg/web/oauthclient/callback.go Outdated Show resolved Hide resolved
pkg/web/oauthclient/callback.go Show resolved Hide resolved
pkg/web/oauthclient/logout.go Outdated Show resolved Hide resolved
@pgalic96 pgalic96 force-pushed the feature/2627-echo-oauth-console-removal branch from fb46a59 to c869669 Compare November 26, 2021 08:15
@github-actions github-actions bot added the c/identity server This is related to the Identity Server label Nov 26, 2021
@adriansmares adriansmares marked this pull request as draft November 26, 2021 12:57
@adriansmares adriansmares changed the title Echo oauthclient/console removal Remove remaining Echo usages Nov 26, 2021
@adriansmares adriansmares changed the base branch from feature/2627-more-echo-removal-v2 to v3.16 November 26, 2021 12:57
@github-actions github-actions bot added dependencies Pull requests that update a dependency file tooling Development tooling labels Nov 26, 2021
@adriansmares adriansmares force-pushed the feature/2627-echo-oauth-console-removal branch 7 times, most recently from 56db800 to c39dc93 Compare November 26, 2021 13:59
@adriansmares adriansmares self-assigned this Nov 26, 2021
@adriansmares adriansmares force-pushed the feature/2627-echo-oauth-console-removal branch 7 times, most recently from 8a0cf8b to a6d4e22 Compare November 26, 2021 14:39
@adriansmares adriansmares force-pushed the feature/2627-echo-oauth-console-removal branch 2 times, most recently from 4a5159e to e39e618 Compare November 29, 2021 14:46
@adriansmares adriansmares force-pushed the feature/2627-echo-oauth-console-removal branch from e39e618 to f005f6d Compare November 29, 2021 14:56
@adriansmares adriansmares marked this pull request as ready for review November 29, 2021 15:40
@adriansmares
Copy link
Contributor

This is now ready for review, and should be compatible with #4884 (cc @pgalic96 - this PR probably should follow up 4484, but it is compatible - see pkg/webhandlers/error.go for the idea).

@adriansmares adriansmares added this to the v3.16.2 milestone Nov 29, 2021
pgalic96
pgalic96 commented Nov 30, 2021
View reviewed changes
pkg/web/oauthclient/token.go Outdated
Comment on lines 86 to 87
w.Header().Set("Content-Type", "application/json")
if err := json.NewEncoder(w).Encode(struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it not necessary to do w.WriteHeader(http.StatusOK) here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Write automatically does StatusOK if no WriteHeader has been done before. And encode calls Write.

htdvisser
htdvisser reviewed Dec 6, 2021
View reviewed changes
Copy link
Contributor

@htdvisser htdvisser left a comment

Choose a reason for hiding this comment

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

Code-owned files LGTM with some small comments

pkg/oauth/server.go Outdated
Comment on lines 243 to 245
if _, err := w.Write(b); err != nil {
webhandlers.Error(w, r, err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we error on writing to w, we won't be able to write the error either.

pkg/web/oauthclient/logout.go Outdated
Comment on lines 76 to 84
w.Header().Set("Content-Type", "application/json")
if err := json.NewEncoder(w).Encode(struct {
OpLogoutURI string `json:"op_logout_uri"`
}{
OpLogoutURI: u.String(),
})
}); err != nil {
webhandlers.Error(w, r, err)
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should write a webhandlers.JSON func?

kschiffer
kschiffer approved these changes Dec 6, 2021
View reviewed changes
Copy link
Contributor

@kschiffer kschiffer left a comment

Choose a reason for hiding this comment

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

LGTM and indeed, end-to-end tests would catch any critical thing.

@github-actions github-actions bot added the c/gateway conf server This is related to the Gateway Configuration Server label Dec 6, 2021
@adriansmares adriansmares merged commit e23c090 into v3.16 Dec 8, 2021
@adriansmares adriansmares deleted the feature/2627-echo-oauth-console-removal branch December 8, 2021 09:25
@adriansmares adriansmares mentioned this pull request Nov 9, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adriansmares adriansmares adriansmares left review comments

@kschiffer kschiffer kschiffer approved these changes

@htdvisser htdvisser htdvisser approved these changes

@bafonins bafonins Awaiting requested review from bafonins

@johanstokking johanstokking Awaiting requested review from johanstokking

@michalborkowski96 michalborkowski96 Awaiting requested review from michalborkowski96

@KrishnaIyer KrishnaIyer Awaiting requested review from KrishnaIyer

@ryaplots ryaplots Awaiting requested review from ryaplots

Assignees

@pgalic96 pgalic96

@adriansmares adriansmares

Labels
c/console This is related to the Console c/gateway conf server This is related to the Gateway Configuration Server c/identity server This is related to the Identity Server dependencies Pull requests that update a dependency file tooling Development tooling ui/web This is related to a web interface
Projects
None yet
Milestone
v3.16.2
Development

Successfully merging this pull request may close these issues.

Remove dependency on Echo
4 participants
@pgalic96 @adriansmares @htdvisser @kschiffer