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

refactor backend #58

Merged
merged 28 commits into from
Sep 10, 2021
Merged

refactor backend #58

merged 28 commits into from
Sep 10, 2021

Conversation

Rampage1xx
Copy link
Contributor

@Rampage1xx Rampage1xx commented Feb 4, 2021 •
edited
Loading

Hello,

This PR aims to improve golang backend code without mutating the folder structure.

phisco
phisco suggested changes Feb 5, 2021
View reviewed changes
Copy link

@phisco phisco left a comment

Choose a reason for hiding this comment

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

minor fixes

internal/kubeconfig/serviceaccount.go Outdated Show resolved Hide resolved
internal/server/handlers.go Outdated Show resolved Hide resolved
internal/server/handlers.go Outdated Show resolved Hide resolved
internal/server/handlers.go Outdated Show resolved Hide resolved
internal/server/handlers.go Outdated Show resolved Hide resolved
internal/server/handlers.go Outdated Show resolved Hide resolved
internal/server/handlers.go Outdated Show resolved Hide resolved
@Rampage1xx Rampage1xx requested review from lzecca78 and removed request for phisco July 9, 2021 12:19
@Rampage1xx Rampage1xx marked this pull request as ready for review July 9, 2021 12:40
@angelbarrera92
Copy link
Contributor

@Rampage1xx Branch need a rebase.
@nandajavarma can you review this PR?

@angelbarrera92 angelbarrera92 removed their request for review July 23, 2021 08:58
lzecca78
lzecca78 approved these changes Sep 8, 2021
View reviewed changes
Copy link

@lzecca78 lzecca78 left a comment

Choose a reason for hiding this comment

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

LGTM ❤️

@angelbarrera92
Copy link
Contributor

How confident we are to push this PR to master? Can we rebase it? Can we have a quick demo around the refactor?

@angelbarrera92 angelbarrera92 added the help wanted Extra attention is needed label Sep 8, 2021
@Rampage1xx
Copy link
Contributor Author

Hello,

In my opinion, without extensive (manual) tests it shouldn't be merged.

Happy to give a demo if needed around the code refactoring

@ipedrazas
Copy link

If extensive manual testing is required, the PR is not ready.

@Rampage1xx
Copy link
Contributor Author

Rampage1xx commented Sep 8, 2021 •
edited
Loading

The existing e2e tests cover a small scope of use-cases and don't give enough confidence to merge in my opinion without manual testing.

I propose to add more e2e and unit-test to make it more resilient so we don't have to do manual checks.

@angelbarrera92 angelbarrera92 merged commit 217216a into master Sep 10, 2021
@angelbarrera92 angelbarrera92 deleted the refactor/go-backend branch September 10, 2021 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phisco phisco phisco requested changes

@lzecca78 lzecca78 lzecca78 approved these changes

@nandajavarma nandajavarma Awaiting requested review from nandajavarma

@nutellinoit nutellinoit Awaiting requested review from nutellinoit

@Al-Pragliola Al-Pragliola Awaiting requested review from Al-Pragliola

@Reuenthal767 Reuenthal767 Awaiting requested review from Reuenthal767

@ipedrazas ipedrazas Awaiting requested review from ipedrazas

Assignees

@nandajavarma nandajavarma

Labels
help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Rampage1xx @angelbarrera92 @ipedrazas @lzecca78 @phisco @nandajavarma