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

fix: do not lowercase values #3

Merged
merged 4 commits into from
Aug 10, 2021
Merged

fix: do not lowercase values #3

merged 4 commits into from
Aug 10, 2021

Conversation

gr2m
Copy link
Collaborator

@gr2m gr2m commented Aug 10, 2021

I updated the e2e test to support E2E_USER_2FA so that I could test with my 2FA enabled test user account. That worked, but the test never completed, and I'm not sure why. So I coudln't verify that the test actually passed locally on my machine. But I did update the snapshots and test manually according with the issue.

closes #1

Copy link
Owner

@stefanbuck stefanbuck left a comment

Choose a reason for hiding this comment

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

I'm glad that you figured out the testing part. I forgot to mention that the current test setup is a bit clunky. This is my first GitHub Action and since the underlying feature is still in beta, I thought it's a good idea to use a End 2 End test setup. The original idea was to use a user/password auth (a test user without 2fa enabled), but this didn't worked on CI because it was asking for a device authentication code during the CI run. Therefore I wonder how your setup is supposed to work, given it needs a 2fa code at runtime so to say. I guess a backup code could work, but only once, right?

Anway, thanks for the contribution and the change looks good to me.

@gr2m
Copy link
Collaborator Author

gr2m commented Aug 10, 2021

Therefore I wonder how your setup is supposed to work, given it needs a 2fa code at runtime so to say

I don't think a GitHub login would ever work on CI. But you could run it locally. Maybe add a simple e2e test to test the whole stack, but create integration tests for the rest

@stefanbuck
Copy link
Owner

I don't think a GitHub login would ever work on CI. But you could run it locally. Maybe add a simple e2e test to test the whole stack, but create integration tests for the rest

Ahhh I see, I thought you made to 2FA login work on CI and I couldn't figure it out how. Right now I do run the tests locally with my test account (without 2fa), so thanks for this awesome contribution. As said, the test setup needs some further thinking but I'm quite busy with another GitHub related project ... https://jumpcat.dev (a command palette for GitHub)

@gr2m
Copy link
Collaborator Author

gr2m commented Aug 10, 2021

could you merge this fix in for the time being?

@stefanbuck stefanbuck merged commit 2fc7695 into stefanbuck:main Aug 10, 2021
@stefanbuck
Copy link
Owner

You're welcome 🙂

@gr2m gr2m deleted the 1/do-not-lowercase-values branch August 10, 2021 19:14
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.

Values are all lowercased
2 participants