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

fix: add warning about python sdk login_user breaking change in 21.4.0 #579

Merged
merged 1 commit into from
Apr 13, 2021
Merged

fix: add warning about python sdk login_user breaking change in 21.4.0 #579

merged 1 commit into from
Apr 13, 2021

Conversation

joeldodge79
Copy link
Contributor

No description provided.

@google-cla google-cla bot added the cla: yes label Apr 12, 2021
@github-actions

This comment has been minimized.

Sign in to view
@github-actions github-actions bot removed the CI:TEST label Apr 12, 2021
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
jkaster
jkaster previously approved these changes Apr 12, 2021
View reviewed changes
Copy link
Contributor

@jkaster jkaster left a comment

Choose a reason for hiding this comment

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

LGTM except for failing CI checks

packages/sdk-codegen/src/python.gen.ts
Comment on lines +414 to +418
if (method.name === 'login_user') {
methodCall =
`${indent}warnings.warn("login_user behavior changed significantly ` +
`in 21.4.0. See https://git.io/JOtH1")\n${methodCall}`
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Want to create an issue to remove this after 3 - 6 months?

@github-actions

This comment has been minimized.

Sign in to view
@joeldodge79 joeldodge79 force-pushed the joeldodge-python-warn-login-user branch from b0b1a8c to 6d11310 Compare April 12, 2021 21:03
jkaster
jkaster previously approved these changes Apr 12, 2021
View reviewed changes
Copy link
Contributor

@jkaster jkaster left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@joeldodge79 joeldodge79 force-pushed the joeldodge-python-warn-login-user branch from 99331b4 to 31d4676 Compare April 12, 2021 21:33
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
jkaster
jkaster previously approved these changes Apr 13, 2021
View reviewed changes
josephaxisa
josephaxisa reviewed Apr 13, 2021
View reviewed changes
Copy link
Contributor

@josephaxisa josephaxisa left a comment

Choose a reason for hiding this comment

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

The Python part lgtm. Maybe there was a conversation I missed, but why is the example miner change part of this PR?

@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@joeldodge79 joeldodge79 force-pushed the joeldodge-python-warn-login-user branch from 306cafb to 2dcf87f Compare April 13, 2021 17:09
@github-actions

This comment has been minimized.

Sign in to view
@joeldodge79 joeldodge79 force-pushed the joeldodge-python-warn-login-user branch 2 times, most recently from aa0268f to ea5f8b3 Compare April 13, 2021 17:23
jkaster
jkaster previously approved these changes Apr 13, 2021
View reviewed changes
Copy link
Contributor

@jkaster jkaster left a comment

Choose a reason for hiding this comment

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

Nice detective work

.github/workflows/codegen-ci.yml Outdated
@@ -23,8 +23,10 @@ on:

env:
LOOKERSDK_BASE_URL: https://localhost:20000
LOOKERSDK_VERIFY_SSL: false
LOOKERSDK_VERIFY_SSL: False
Copy link
Contributor

Choose a reason for hiding this comment

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

case sensitivity for the ... win?

.github/workflows/codegen-ci.yml Outdated
with:
node-version: ${{ matrix.node-version }}
node-version: 15.x
Copy link
Contributor

Choose a reason for hiding this comment

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

that'll speed things up

.github/workflows/codegen-ci.yml Outdated
echo "base_url=https://self-signed.looker.com:19999" >> looker.ini
echo "verify_ssl=False" >> looker.ini
echo "base_url=https://localhost:20000" >> looker.ini
echo "verify_ssl=false" >> looker.ini
Copy link
Contributor

Choose a reason for hiding this comment

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

our SDKs should treat false and False identically FWIW

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 could almost swear that the path it takes in fetchSpec (where you just ApiConfig it) it was failing with False and then passed with false.

packages/sdk-codegen-scripts/src/exampleMiner.ts Outdated
Comment on lines 157 to 164
if (!match) {
// git origin on CI: https://github.com/looker-open-source/sdk-codegen
const httpExtractor = /(https:\/\/github.com.*)(|.git)/
match = httpExtractor.exec(origin)
if (!match) {
return ''
}
return match[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

I bet this was fun to track down

packages/sdk-codegen-scripts/src/fetchSpec.ts Outdated
Comment on lines 115 to 116
client_id: props.client_id ?? process.env.LOOKERSDK_CLIENT_ID,
client_secret: props.client_secret ?? process.env.LOOKERSDK_CLIENT_SECRET,
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to fix that config resolution downstream in the future

@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@joeldodge79 joeldodge79 force-pushed the joeldodge-python-warn-login-user branch from ea5f8b3 to 8c5bb53 Compare April 13, 2021 17:44
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@joeldodge79 joeldodge79 merged commit c74d447 into main Apr 13, 2021
@joeldodge79 joeldodge79 deleted the joeldodge-python-warn-login-user branch April 13, 2021 22:06
@github-actions

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Contributor

Test Results

  10 files  +    9    10 suites  +4   2m 9s ⏱️ + 1m 37s
140 tests +  82  140 ✔️ +  94  0 💤  - 12  0 ❌ ±0 
788 runs  +730  788 ✔️ +742  0 💤  - 12  0 ❌ ±0 

Results for commit c74d447. ± Comparison against base commit 57f6351.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josephaxisa josephaxisa josephaxisa left review comments

@jkaster jkaster jkaster approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joeldodge79 @jkaster @josephaxisa