Skip to content

Conversation

@maxhora
Copy link
Contributor

@maxhora maxhora commented Feb 10, 2020

No description provided.

@shcheklein
Copy link
Member

looks good! did you manage to run tests that are included in the library? can we fix/add a test for this?

@maxhora
Copy link
Contributor Author

maxhora commented Feb 15, 2020

@shcheklein some tests are failing, will look into it

@maxhora maxhora changed the title Allow service account auth from saved credentials without specified p… Service Account auth from saved credentials; Fix and run tests on Travis. Feb 23, 2020
@shcheklein
Copy link
Member

@Maxris looks like a bunch of png1, png2 files were committed, do we need them?

Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Guys, do we really need binary png files here? They will live in the history forever.

@maxhora
Copy link
Contributor Author

maxhora commented Feb 24, 2020

Guys, do we really need binary png files here? They will leave in the history forever.

these 2 files - "a.png" and "b.png" are expected by tests to be in place, but it seems we can replace them with any dynamically generate pair of different(between each other) files.

Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

LGTM

@maxhora maxhora force-pushed the service-account-keyless-auth branch 2 times, most recently from 4a61d4b to d13ff2e Compare February 26, 2020 17:06
@maxhora
Copy link
Contributor Author

maxhora commented Feb 26, 2020

@efiop @shcheklein formatting checks are added in separate commit, should be good to merge all together

@shcheklein
Copy link
Member

@Maxris could we please revert the formatting? It's impossible now to review anything. We can apply it later. That why I wanted to make it as a separate step. We have way too many things at once in this PR now.

@efiop
Copy link
Contributor

efiop commented Feb 26, 2020

@Maxris @shcheklein Sorry for the confusion, guys 🙁

@maxhora
Copy link
Contributor Author

maxhora commented Feb 26, 2020

@Maxris could we please revert the formatting? It's impossible now to review anything. We can apply it later. That why I wanted to make it as a separate step. We have way too many things at once in this PR now.

@shcheklein I can revert last commit, no problem, but idea was that all source code changes not related to the formatting cheks are only in the first commit bcb5e2b which you can review and add comments by above link.

@shcheklein
Copy link
Member

@Maxris ah, okay, I see. Will it be easy for you to support this structure going forward? Up to you.

@maxhora
Copy link
Contributor Author

maxhora commented Feb 26, 2020

@shcheklein I supposed only minor further changes to the codebase in scope of this PR and make 3rd commit with them :) But if there will be a lot things to change, then better to revert formatting check commit :)

@shcheklein
Copy link
Member

@Maxris sure, let me review the commit then and we can decide after that :)

Copy link

@Suor Suor left a comment

Choose a reason for hiding this comment

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

I would say this ran out of scope, we should try to not make that even less unmanageable, i.e. finish the core and merge. Other ideas might be applied later.

@maxhora
Copy link
Contributor Author

maxhora commented Feb 27, 2020

going to backup code formatting changes in remote branch enable-code-formatting-checks, will remove these changes from this PR

@maxhora maxhora force-pushed the service-account-keyless-auth branch from d13ff2e to b63ab7f Compare February 27, 2020 17:07
@maxhora
Copy link
Contributor Author

maxhora commented Feb 27, 2020

@shcheklein @Suor could you please check if it looks good now, thanks

@shcheklein shcheklein merged commit 56ddfaa into master Feb 28, 2020
@shcheklein shcheklein deleted the service-account-keyless-auth branch November 15, 2020 18:00
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.

5 participants