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(rosetta): 'extract' does not translate samples in submodule READMEs #2712

Merged
merged 7 commits into from
Mar 18, 2021
Merged

fix(rosetta): 'extract' does not translate samples in submodule READMEs #2712

merged 7 commits into from
Mar 18, 2021

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Mar 16, 2021 •
edited by iliapolo
Loading

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@rix0rrr rix0rrr self-assigned this Mar 16, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Mar 16, 2021
@rix0rrr rix0rrr removed the request for review from MrArnoldPalmer March 16, 2021 14:16
@rix0rrr
Copy link
Contributor Author

rix0rrr commented Mar 18, 2021

I don't understand why the integ test is failing

@RomainMuller
Copy link
Contributor

There is an unhandled promise rejection in jsii-rosetta (it exists successfully, but really failed):

UnhandledPromiseRejectionWarning: Error: Sample uses fixture with-lambda-trigger, but not found: /home/runner/work/jsii/jsii/aws-cdk/packages/aws-cdk-lib/rosetta/with-lambda-trigger.ts-fixture

It appears some CDK documentation block uses a non-existent fixture file!

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Mar 18, 2021

Thanks for digging that one out for me!

Guess there's no getting around the fact that the strategies Rosetta uses to get at fixtures and types aren't going to work for a monocdk that has been stitched together from the individual source packages.

Here's what Rosetta sees:

  • Here we are extracting all the samples from .../packages/aws-cdk-lib.
  • Some submodule has a README that uses a fixture named with-lambda-trigger.

Here's what Rosetta needs to conclude:

  • Oooooh OF COURSE I know that file lives in .../packages/aws-cdk-lib/lib/aws-lambda/rosetta/with-lambda-trigger.ts.

....but how? I don't think it can.

Bottom line: this can't ever work without a lot more investment.

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Mar 18, 2021

Supposedly we can move all fixtures to the right location... but they'll still have the wrong import names.

@rix0rrr rix0rrr mentioned this pull request Mar 18, 2021
Merged
mergify bot pushed a commit to aws/aws-cdk that referenced this pull request Mar 18, 2021
@rix0rrr
chore(ubergen): combine all Rosetta fixtures (#13670)
e047bda 
We need this to have `jsii-rosetta` runs that try to translate READMEs
from submodules not crash (because it can't find the referenced fixture
files).

This is not a correct final state yet, but it makes the build not crash.

Hopefully allows aws/jsii#2712 to be merged.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@mergify
Copy link
Contributor

mergify bot commented Mar 18, 2021

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Mar 18, 2021
@mergify
Copy link
Contributor

mergify bot commented Mar 18, 2021

Merging (with squash)...

@mergify
Copy link
Contributor

mergify bot commented Mar 18, 2021

Merging (with squash)...

@mergify mergify bot merged commit ccac1bd into main Mar 18, 2021
@mergify mergify bot deleted the huijbers/rosetta-submodules branch March 18, 2021 18:37
@mergify
Copy link
Contributor

mergify bot commented Mar 18, 2021

Merging (with squash)...

@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Mar 18, 2021
iliapolo added a commit that referenced this pull request Mar 22, 2021
@iliapolo
Revert "fix(rosetta): 'extract' does not translate samples in submodu…
670049f 
…le READMEs (#2712)"

This reverts commit ccac1bd.
@iliapolo iliapolo mentioned this pull request Mar 22, 2021
Merged
mergify bot pushed a commit that referenced this pull request Mar 22, 2021
@iliapolo
revert(rosetta): 'extract' does not translate samples in submodule RE…
d3cadb8 
…ADMEs (#2729)

Reverts #2712

The mentioned commit breaks our cdk integ test since it depends on aws/aws-cdk#13670, which hasn't landed on the release branch yet. 

This will be reintroduced after we release CDK.
eladb pushed a commit to aws/aws-cdk that referenced this pull request Mar 24, 2021
@rix0rrr
chore(ubergen): combine all Rosetta fixtures (#13670)
3b1c52f 
We need this to have `jsii-rosetta` runs that try to translate READMEs
from submodules not crash (because it can't find the referenced fixture
files).

This is not a correct final state yet, but it makes the build not crash.

Hopefully allows aws/jsii#2712 to be merged.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@iliapolo iliapolo restored the huijbers/rosetta-submodules branch March 25, 2021 14:36
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
@rix0rrr @hollanddd
chore(ubergen): combine all Rosetta fixtures (aws#13670)
1f2e5b3 
We need this to have `jsii-rosetta` runs that try to translate READMEs
from submodules not crash (because it can't find the referenced fixture
files).

This is not a correct final state yet, but it makes the build not crash.

Hopefully allows aws/jsii#2712 to be merged.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RomainMuller RomainMuller RomainMuller approved these changes

Assignees

@rix0rrr rix0rrr

Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rix0rrr @RomainMuller