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

AEROGEAR-2850 New UPS annotations on mobile client #38

Merged
merged 6 commits into from
Jun 7, 2018

Conversation

aliok
Copy link
Member

@aliok aliok commented May 25, 2018

JIRA: https://issues.jboss.org/browse/AEROGEAR-2850

Should be verified along with aerogear-attic/origin-web-console#50, thus wrote the verification instructions there.

How to make this branch deployed on OpenShift:

  1. Just provision regular UPS APB
  2. Build this branch, push it your Docker org
  3. Change the deployment config for configurator on OpenShift to use your image

Summary of changes:

  • Wanted to introduce some mock testing, as integration tests would be super hard.
  • So, divided main.go file info multiple mockable types
  • Most important business logic now lies in configOperator.go file and it is tested with an OK coverage
  • The real work, annotation work, is done afterwards as I was afraid of breaking things.
  • Annotations are not handled in one place, which made things easier when modifying them.

@aliok aliok force-pushed the AEROGEAR-2850-new-ups-annotations branch 4 times, most recently from 074161b to a35566d Compare May 31, 2018 12:14
@aliok aliok changed the title [WIP] AEROGEAR-2850 New UPS annotations on mobile client AEROGEAR-2850 New UPS annotations on mobile client Jun 1, 2018
@aliok aliok requested review from pb82 and darahayes June 1, 2018 09:03
@aliok
Copy link
Member Author

aliok commented Jun 1, 2018

cc @pb82 mind verifying this along with the UI changes?

@darahayes
Copy link

@aliok I haven't done verification steps but from a code point of view I'm so happy with this PR. Fantastic work! 👏

@pb82
Copy link
Contributor

pb82 commented Jun 1, 2018

@aliok when running make setup i get this error:

mockery -all -inpkg -dir pkg
make: mockery: No such file or directory
make: *** [setup] Error 1

anything else i need to do first?

@pb82
Copy link
Contributor

pb82 commented Jun 1, 2018

@aliok nvm. i had to run go get github.com/vektra/mockery/.../ first, then it worked.

@pb82
Copy link
Contributor

pb82 commented Jun 4, 2018

@aliok I'm getting this errors when running the tests:

--- FAIL: TestConfigOperator_handleAddSecret_whenAndroid_andNoVariantExistsWithSameGoogleKey (0.00s)
panic: 
assert: mock: I don't know what to return because the method call was unexpected.
        Either do Mock.On("getPushApplicationName").Return(...) first, or remove the getPushApplicationName() call.
        This method was unexpected:
                getPushApplicationName()
                
        at: [mock_UpsClient.go:122 configOperator.go:425 configOperator.go:257 configOperator.go:94 configOperator_test.go:242] [recovered]
        panic: 
assert: mock: I don't know what to return because the method call was unexpected.
        Either do Mock.On("getPushApplicationName").Return(...) first, or remove the getPushApplicationName() call.
        This method was unexpected:
                getPushApplicationName()
                
        at: [mock_UpsClient.go:122 configOperator.go:425 configOperator.go:257 configOperator.go:94 configOperator_test.go:242]

I did create the mock objects before running this. Any idea?

@aliok
Copy link
Member Author

aliok commented Jun 4, 2018

@pb82 it seems I never had the tests part of CircleCI build, so when I forgot to adapt them for the latest changes, I didn't get any notification.
all green now @pb82

Copy link
Contributor

@pb82 pb82 left a comment

Choose a reason for hiding this comment

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

Verified locally

aliok added 6 commits June 7, 2018 00:39
Introduce variant type label in the annotations for a better labeling

Remove all push related annotations on mobile client when there is no variant anymore
It can be retrieved from other metadata in UI
@aliok aliok force-pushed the AEROGEAR-2850-new-ups-annotations branch from 45ac3fc to aaea9ae Compare June 6, 2018 21:47
@aliok
Copy link
Member Author

aliok commented Jun 6, 2018

@pb82 what's the reason that checking an existing Android variant with the same Google key is removed?

Anyway, I rebased my branch, squashed commits. But I didn't want to create a single commit because of file restructuring.

Give me an +1 and I will merge.

@pb82
Copy link
Contributor

pb82 commented Jun 7, 2018

@aliok The reason was that this check doesn't make a lot of sense: we only did it for Android because iOS doesn't have a unique property besides the variant id that we could use. And you can have one variant of each kind anyway. It was a leftover from the early days of the Operator when we only Supported Android. Also i believe @psturc has some testing setup that requires him to have multiple variants using the same credentials (which is totally fine for UPS).

@aliok aliok merged commit 863e5c9 into aerogear:master Jun 7, 2018
@aliok aliok deleted the AEROGEAR-2850-new-ups-annotations branch June 7, 2018 08:35
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.

3 participants