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

Fixing the connectors build w/ latest cdk models #20743

Merged
merged 2 commits into from
Dec 21, 2022

Conversation

brianjlai
Copy link
Contributor

What

A prior merge didn't have the latest component schema up to date so we need to update the autogenerated code.

How

Ran SUB_BUILD=CONNECTORS_BASE ./gradlew format to regenerate models according to the latest schema and got rid of an unneeded file.

@brianjlai brianjlai requested a review from a team as a code owner December 21, 2022 00:22
@octavia-squidington-iv octavia-squidington-iv added the CDK Connector Development Kit label Dec 21, 2022
@@ -1,918 +0,0 @@
"$schema": http://json-schema.org/draft-07/schema#
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we had renamed the schema to declarative_component_schema.yaml but this file was not removed. Just getting rid of it since it's unnecessary.

Copy link
Contributor

@girarda girarda left a comment

Choose a reason for hiding this comment

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

is gradlew format not part of the pipeline?

@brianjlai
Copy link
Contributor Author

It should be part of CI since its integrated into the base connectors build and why it's failing now. Its possible that it wasn't caught the first time because the previous review was the first one to introduce the CI check. Now that its fully merged, now it will start catching them in all PRs

But as it stands now, the models weren't regenerated against the latest schema most likely. If it were, then the OAuthAuthenticator rename would have been reflected.

@brianjlai
Copy link
Contributor Author

/approve-and-merge reason="fixing a red build for connectors base”

@octavia-approvington
Copy link
Contributor

Myomoto says it looks good
thats a niiice

@brianjlai
Copy link
Contributor Author

weird, octavia is just leaving my PR hanging. I'll try not to take offense

@brianjlai
Copy link
Contributor Author

/approve-and-merge reason="approved but never merged?”

@octavia-approvington
Copy link
Contributor

A crack team of mammals has made a decision.
imagine a seal of approval

Copy link
Contributor

@maxi297 maxi297 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@maxi297
Copy link
Contributor

maxi297 commented Dec 21, 2022

There was a message saying the branch was no up-to-date with master. Not sure if it could help but I did merge master in this branch and will try to approve-and-merge again

@maxi297
Copy link
Contributor

maxi297 commented Dec 21, 2022

/approve-and-merge reason="fixing a red build for connectors base”

@octavia-approvington
Copy link
Contributor

This looks fine!
Merged!
Imagine it being fine

@rodireich rodireich self-requested a review December 21, 2022 16:55
@rodireich
Copy link
Contributor

/approve-and-merge reason="Because Maxime asked"

@octavia-approvington
Copy link
Contributor

Approve this
imagine the commander saying yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants