-
Notifications
You must be signed in to change notification settings - Fork 192
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
feat: SDK build for 22.0. #959
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
APIX Tests0 files 0 suites 0s ⏱️ Results for commit 20fca545. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel pretty strongly that we should cleanup the current state of the past couple releases and release #930 as a patch before we merge 22.0 in
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
APIX Tests0 files 0 suites 0s ⏱️ Results for commit 3a755020. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
0dd15a1
to
efb5d67
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
fd6355b
to
563f89c
Compare
This comment has been minimized.
This comment has been minimized.
8bf3b6c
to
c35042a
Compare
This comment has been minimized.
This comment has been minimized.
c35042a
to
b3e841c
Compare
This comment has been minimized.
This comment has been minimized.
e1fdabe
to
a43da3c
Compare
Release-As: 1.22.0
69ff957
to
98e9bcf
Compare
Typescript Tests 6 files 75 suites 3m 16s ⏱️ Results for commit 98e9bcf. |
Codegen Tests 1 files 17 suites 30s ⏱️ Results for commit fe94373. |
Python Tests 9 files 9 suites 1m 55s ⏱️ Results for commit fe94373. |
Typescript Tests 6 files 75 suites 3m 14s ⏱️ Results for commit fe94373. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the python generated code changes and it seems good to me. I briefly looked at the workflow file changes and they seem reasonable too though I haven't fully grokked all the changes.
One change we need can be accomplished in two ways.
- separate out the "update CI for 22.0" changes (workflows etc) from the codegen for 22.0 (generated language files) into a separate PR and merge them first - that seems to be the intention with the title of this PR:
chore: SDK build for 22.0
- leave it all combined as is BUT update the title to
feat: Looker 22.0 release
(otherwise release-please will NOT pickup the fact that there are user facing changes in this PR becausechore
is not considered user facing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with Joel's feedback. Nothing further to add right now.
@joeldodge79 I renamed it to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on moving the CI noise-reduction effort forward!!
I'll approve this but with a note: we don't have a good mechanism for testing previous sdk versions against newer Looker versions. And maybe that's not a big deal - if something breaks then upgrade to latest SDK which DO test against all previously supported Looker versions. However, in the old workflow where we modified CI separately and beforehand to test against the next Looker release, we had at least a transient rolling window of "SDK for Looker version N works against Looker version N+1". If the generated code changes for 22.0 are merged together with the CI changes to test against 22.0 then we lose that for SDK(21.20) vs Looker(22.0). But I don't think it's critical, just pointing it out. That and my manta of keeping a PR as single purpose as possible (update CI vs generate new sdk)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
OK. I'll do that next time. |
No description provided.