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(aws-machine-image): lazily load ec2client #13437

Merged
merged 4 commits into from
Jan 9, 2022
Merged

Conversation

JamieMagee
Copy link
Contributor

@JamieMagee JamieMagee commented Jan 9, 2022

Changes:

  • Unifying the versions of @aws-sdk/* packages
  • Introducing the Lazy class for lazily loading expensive calls
  • Lazily loading EC2Client in the constructor for AwsMachineImageDataSource

Context:

I attempted a couple of different fixes for this:

  • Mocking HTTP calls with nock
  • Setting up mockClient from aws-sdk-client-mock globally
  • Creating a heap dump and examining it

In the end, lazily loading EC2Client was the only fix I could get to work reliably, with the smallest code changes. I also think that Lazy could come in useful elsewhere in the codebase.

Closes #13389

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@JamieMagee JamieMagee changed the title Bump aws-sdk to matching version fix(aws-machine-image): lazily load ec2client Jan 9, 2022
@JamieMagee
Copy link
Contributor Author

I can't request a review from @markussiebert, but tagging for visibility as you created this datasource.

@JamieMagee JamieMagee force-pushed the fix/aws-sdk-client-lazy branch 2 times, most recently from 4b969a3 to b905176 Compare January 9, 2022 02:29
viceice
viceice previously approved these changes Jan 9, 2022
rarkins
rarkins previously approved these changes Jan 9, 2022
@JamieMagee JamieMagee dismissed stale reviews from rarkins and viceice via aecfcaf January 9, 2022 16:47
@JamieMagee JamieMagee enabled auto-merge (squash) January 9, 2022 16:47
@JamieMagee
Copy link
Contributor Author

Rebased and accidentally invalidated the reviews 😭

@JamieMagee JamieMagee merged commit e3d213e into main Jan 9, 2022
@JamieMagee JamieMagee deleted the fix/aws-sdk-client-lazy branch January 9, 2022 17:57
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 31.21.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Comment on lines +131 to +132
"@aws-sdk/client-ec2": "3.46.0",
"@aws-sdk/client-ecr": "3.46.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we properly grouping the @aws-sdk/client-ec2, @aws-sdk/client-ecr and @aws-sdk/client-s3 packages in our renovatebot/.github rules, or in our own repo's renovate.json file?

Is there a way to prevent these packages to get out of sync with each other?

Copy link
Member

Choose a reason for hiding this comment

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

@HonkingGoose hey are grouped, but they need dashborad aproval. They got out of sync, because some are added later in a PR and the main branch was already on a newer version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@HonkingGoose they are grouped, but they need dashborad aproval. They got out of sync, because some are added later in a PR and the main branch was already on a newer version.

Are you sure these packages are grouped together???

Quote from our renovatebot/github/default.json 1 file:

    {
      "description": "Require approval for aws-sdk as it updates too often",
      "matchDatasources": ["npm"],
      "matchPackageNames": ["aws-sdk"],
      "matchPackagePatterns": ["^@aws-sdk\\/"],
      "dependencyDashboardApproval": true
    },

I do not see anything here that actually groups the aws-sdk packages together?

Footnotes

  1. https://github.com/renovatebot/.github/blob/03cb56ff31a80b78b224e6024508007f502d77b6/default.json#L80-L86

Copy link
Member

Choose a reason for hiding this comment

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

yes:

'aws-sdk-js-v3': 'https://github.com/aws/aws-sdk-js-v3',

Copy link
Collaborator

Choose a reason for hiding this comment

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

That line only groups any aws-sdk-js-v3 packages, I think.

Would it be a good idea to have a patternGroups entry that puts these packages into one group?

  • @aws-sdk/client-ec2
  • @aws-sdk/client-ecr
  • @aws-sdk/client-s3

const patternGroups = {
babel6: '^babel6$',
clarity: ['^@cds/', '^@clr/'],
wordpress: '^@wordpress/',
angularmaterial: ['^@angular/material', '^@angular/cdk'],
'aws-java-sdk': '^com.amazonaws:aws-java-sdk-',
'aws-java-sdk-v2': '^software.amazon.awssdk:',
embroider: '^@embroider/',
fullcalendar: '^@fullcalendar/',
};

Copy link
Member

Choose a reason for hiding this comment

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

@HonkingGoose They are grouped via sourceUrl, checkout https://www.npmjs.com/package/@aws-sdk/client-ec2. All @aws-sdk/* js pacvkages are published from https://github.com/aws/aws-sdk-js-v3

Lets move this to a discussion if you need further clarification.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jest test suite hangs indefinitely
5 participants