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

Add ConsoleLink Creation/Remove #448

Closed
wants to merge 2 commits into from
Closed

Add ConsoleLink Creation/Remove #448

wants to merge 2 commits into from

Conversation

rashelrr
Copy link
Contributor

@rashelrr rashelrr commented Aug 10, 2020 •
edited
Loading

Added ConsoleLink Creation/Remove
Added go mod vendor folder

@rashelrr rashelrr force-pushed the consolelink branch 4 times, most recently from 5cc1e17 to 773644e Compare August 10, 2020 22:09
bmozaffa
bmozaffa suggested changes Aug 10, 2020
View reviewed changes
Copy link
Contributor

@bmozaffa bmozaffa left a comment

Choose a reason for hiding this comment

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

Nothing major, but a few suggestions

deploy/cluster_role.yaml Outdated Show resolved Hide resolved
pkg/controller/apimanager/apimanager_controller.go Outdated Show resolved Hide resolved
pkg/controller/apimanager/apimanager_controller.go Outdated Show resolved Hide resolved
pkg/controller/apimanager/apimanager_controller.go Outdated Show resolved Hide resolved
pkg/controller/apimanager/apimanager_controller.go Outdated Show resolved Hide resolved
pkg/helper/webconsole.go Outdated Show resolved Hide resolved
pkg/helper/webconsole.go Outdated Show resolved Hide resolved
pkg/helper/webconsole.go Outdated
err := c.Get(ctx, types.NamespacedName{Name: consoleLinkName}, consoleLink)
if route != nil {
if err != nil && apierrors.IsNotFound(err) {
consoleLink = createNamespaceDashboardLink(consoleLinkName, route, apimanager)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a confusing function name. Makes me think you create it, whereas you apparently only create a local object, and create it later in the reconcile call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which function name are you referring to?

Copy link
Contributor

Choose a reason for hiding this comment

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

createNamespaceDashboardLink, in that it only creates an object in memory. Not a big deal, but maybe try to make that clear in the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm basing my code off of other consolelink creation examples (Teiid, Apicurio) to be consistent, but I can change the name no problem

Copy link
Contributor

Choose a reason for hiding this comment

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

This one is not a big deal, so you could go either way, but as far as consistency, it only matters that the code is consistent with other parts of this operator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! Function name is now createConsoleLinkPointer.

pkg/apis/addtoscheme_apps_v1alpha1.go Outdated Show resolved Hide resolved
pkg/helper/webconsole.go Outdated Show resolved Hide resolved
@codeclimate
Copy link

codeclimate bot commented Aug 11, 2020

Code Climate has analyzed commit 84e48fc and detected 0 issues on this pull request.

View more on Code Climate.

@rashelrr rashelrr changed the title [WIP] Add ConsoleLink Creation/Remove Add ConsoleLink Creation/Remove Aug 11, 2020
@rashelrr rashelrr changed the title Add ConsoleLink Creation/Remove [WIP] Add ConsoleLink Creation/Remove Aug 11, 2020
bmozaffa
bmozaffa approved these changes Aug 11, 2020
View reviewed changes
Copy link
Contributor

@bmozaffa bmozaffa left a comment

Choose a reason for hiding this comment

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

Great job, LGTM!

@rashelrr rashelrr changed the title [WIP] Add ConsoleLink Creation/Remove Add ConsoleLink Creation/Remove Aug 11, 2020
@miguelsorianod
Copy link
Contributor

Hi,

Thanks for your contribution.

We are currently evaluating/analysing the functionality and the pull request and we will come back to you as soon as possible with some feedback

@eguzki eguzki self-assigned this Aug 25, 2020
@eguzki
Copy link
Member

eguzki commented Aug 26, 2020

@rashelrr before going into deep review, two quick comments:

  • Do not add vendoring to the repo. go.mod file provides reproducible builds and adding vendor files is not necessary at all.
  • Do not update go.mod file unless it is necessary. I do not see any new dep added or removed. Did you need to "upgrade" existing dep? If so, could you explain briefly which one and why?

@bmozaffa
Copy link
Contributor

@eguzki Rashel was our summer intern who completed her internship last week. @myeung18 worked with her on this and other PRs, and would be able to pick this up and drive it home. However, he is on PTO this week and returns next week. Can we leave this PR alone and neither merge or close it, and let Marco open a new PR on top of these commits? That way, he can address these and other feedbacks in a new commit, and when resolved and merged, this PR will also automatically be marked as merged, so we can get Rashel's commits in. Does that work?

@eguzki
Copy link
Member

eguzki commented Aug 27, 2020

Sure @bmozaffa
Thnx for the info.

@eguzki eguzki removed their assignment Aug 27, 2020
@myeung18
Copy link

myeung18 commented Sep 4, 2020 •
edited
Loading

@eguzki Rashel was our summer intern who completed her internship last week. @myeung18 worked with her on this and other PRs, and would be able to pick this up and drive it home. However, he is on PTO this week and returns next week. Can we leave this PR alone and neither merge or close it, and let Marco open a new PR on top of these commits? That way, he can address these and other feedbacks in a new commit, and when resolved and merged, this PR will also automatically be marked as merged, so we can get Rashel's commits in. Does that work?

@eguzki @bmozaffa
A new PR#462 is created to replace this one. please close this one.

@myeung18 myeung18 mentioned this pull request Sep 4, 2020
Merged
@eguzki eguzki closed this Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmozaffa bmozaffa bmozaffa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rashelrr @miguelsorianod @eguzki @bmozaffa @myeung18