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

feat: Update stage-deadline script with new version index #139

Merged

grbartel merged 4 commits into aws:mainline from kozlove-aws:consuming_index_file

Oct 8, 2020

Contributor

kozlove-aws commented Sep 29, 2020 •

edited

Loading

Problem:

We need to update our stage-deadline script to use index file with installers URLs, so customers can specify a version, such as 10.1.9 or 10.1.9.2, or latest and not have to enter the URL's themselves, or have the script attempt to form them on its own. The updated script should access the index, parse the JSON, and use that to determine where to download the customer's requested version.

Solution

Was created lambda script that consume index file and return URLs for required product and platform.
Was modified stage-script to use lambda for getting URLs to Deadline client and Docker recipe.

Testing

Were created unit tests for lambda script that covered 100% of functionality.
Was built project and tested that stage-script correctly download deadline client and Docker recipe in next cases:

latest versions without any parameters.
correct version when version is specified
latest possible version when specified partial version (like '10.1' or '10.1.8')

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

Eugene Kozlov added 3 commits

September 16, 2020 23:40


          feat: add version-provider

3b9ed9e


          Merge branch 'mainline' of github.com:aws/aws-rfdk into mainline

896c92e


          feat: update stage-deadline script with new version index

bde1be6

jusiskin added the contribution/core label

grbartel suggested changes

View reviewed changes

packages/aws-rfdk/bin/stage-deadline.ts Show resolved Hide resolved

packages/aws-rfdk/bin/stage-deadline.ts Outdated Show resolved Hide resolved

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/handler.ts Show resolved Hide resolved

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/handler.ts Outdated

+                private implementsIVersionProviderProperties(value: any): boolean {
+                  if (!value || typeof(value) !== 'object') { return false; }
+                  // TODO: Ignore case

Contributor

grbartel Sep 30, 2020

Can you either implement these TODO's or are you going to create issues to fix them.

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/handler.ts Outdated

+                  const productionInfoURL = 'https://downloads.thinkboxsoftware.com/version_info.json';
+                  // eslint-disable-next-line @typescript-eslint/no-require-imports
+                  const parsedUrl = require('url').parse(productionInfoURL);

Contributor

grbartel Sep 30, 2020

do we have to use require imports here? or can we just do a normal import at the top of the file.

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/handler.ts Outdated

+                  return new Promise((resolve, reject) => {
+                    // eslint-disable-next-line @typescript-eslint/no-require-imports
+                    require('https').get(options, (res: IncomingMessage) => {

Contributor

grbartel Sep 30, 2020

Same question for imports.

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/handler.ts Outdated

+                }
+                /**
+                 * This method reads index file.

Contributor

grbartel Sep 30, 2020

This docstring is not very useful since all it says is the name of the function.

What do we do with the read file return it ? throw it out? what do we return it as? a string? json blob?

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/handler.ts Outdated

+                }
+                private parseVersionString(versionString: string): RegExpExecArray | null {
+                  const VALID_VERSION_REGEX = /^(0|[1-9]\d*)(?:\.(0|[1-9]\d*))?(?:\.(0|[1-9]\d*))?(?:\.(0|[1-9]\d*))?$/;

Contributor

grbartel Sep 30, 2020

Since this REGEX is constant could we make this a class level constant.

Also could we use /^(0|[1-9]\d*)(?:.(0|[1-9]\d*)){0,3}$/ instead. the {0,3} means 0-3 copies of the preceeding so we can use this instead of repeating the same section 3 times with the ? quantifier.

Contributor Author

kozlove-aws Oct 1, 2020

Your expression work great for validation but it return only first and last part as result of exec

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/handler.ts Outdated

+              export async function handler(event: CfnRequestEvent, context: LambdaContext): Promise<string> {
+                const versionProvider = new VersionProvider();
+                return await versionProvider.handler(event, context);
+              }

Contributor

grbartel Sep 30, 2020

missing a new line at the end of the file.

horsmand reviewed

View reviewed changes

packages/aws-rfdk/bin/stage-deadline.ts Show resolved Hide resolved

horsmand suggested changes

View reviewed changes

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/test/version-provider.test.ts Outdated Show resolved Hide resolved

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/test/version-provider.test.ts Outdated Show resolved Hide resolved

packages/aws-rfdk/bin/stage-deadline.ts Outdated Show resolved Hide resolved

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/test/version-provider.test.ts Outdated Show resolved Hide resolved

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/test/version-provider.test.ts Show resolved Hide resolved

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/test/version-provider.test.ts Outdated Show resolved Hide resolved

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/test/version-provider.test.ts Outdated Show resolved Hide resolved

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/test/version-provider.test.ts Outdated Show resolved Hide resolved

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/test/version-provider.test.ts Outdated Show resolved Hide resolved

horsmand reviewed

View reviewed changes

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/version-provider.ts Outdated Show resolved Hide resolved

horsmand reviewed

View reviewed changes

packages/aws-rfdk/bin/index-test.json Outdated Show resolved Hide resolved

horsmand reviewed

View reviewed changes

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/version-provider.ts Outdated Show resolved Hide resolved

kozlove-aws force-pushed the consuming_index_file branch from 8906ad8 to 2359f29 Compare

October 4, 2020 19:01

horsmand added the pr/do-not-merge label

Contributor

horsmand commented Oct 5, 2020

Added the do not merge label because this is dependent on the version index file being made available on the downloads website first.

horsmand reviewed

View reviewed changes

packages/aws-rfdk/bin/index-test.json Outdated Show resolved Hide resolved

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/test/version-provider.test.ts Outdated Show resolved Hide resolved

kozlove-aws force-pushed the consuming_index_file branch 4 times, most recently from e1a8396 to 002f8be Compare

October 6, 2020 00:30

horsmand previously approved these changes

View reviewed changes

horsmand removed the pr/do-not-merge label

Contributor

horsmand commented Oct 7, 2020

The index file has been published, I've removed the do not merge label. Everything looks good, approved.

grbartel suggested changes

View reviewed changes

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/version-provider.ts Outdated Show resolved Hide resolved

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/version-provider.ts Outdated Show resolved Hide resolved

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/version-provider.ts Outdated

+                  // and get the matching version from the indexed version map.
+                  for (let versionIndex = 0; versionIndex < 4; versionIndex++) {
+                    let version: string;
+                    if (null === requestedVersion

Contributor

grbartel Oct 7, 2020

can this be changed to using optional chaining something along the lines of

requestedVersion?.[versionIndex + 1] == null

kozlove-aws dismissed horsmand’s stale review via

a5c4f2c

October 7, 2020 17:19

kozlove-aws force-pushed the consuming_index_file branch from 002f8be to a5c4f2c Compare

October 7, 2020 17:19

ddneilson requested changes

View reviewed changes

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/version-provider.ts Outdated

+                }
+                /**
+                 * @inheritdoc

Contributor

ddneilson Oct 7, 2020

inheritdoc from what? This is a base class

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/version-provider.ts Show resolved Hide resolved

packages/aws-rfdk/lib/core/lambdas/nodejs/version-provider/version-provider.ts Show resolved Hide resolved


          fix: move version provider logic in separate script from handler

47b6a7c

kozlove-aws force-pushed the consuming_index_file branch from a5c4f2c to 47b6a7c Compare

October 7, 2020 21:49

ddneilson approved these changes

View reviewed changes

horsmand approved these changes

View reviewed changes

Contributor

horsmand left a comment

Grant's comments are still unresolved, but I can see they've been addressed. Looks good to me!

grbartel approved these changes

View reviewed changes

grbartel merged commit 9cbf99f into aws:mainline

kozlove-aws referenced this pull request in kozlove-aws/aws-rfdk


          feat: Update stage-deadline script with new version index (#139)

fcb926e

* feat: add version-provider

* feat: update stage-deadline script with new version index

* fix: move version provider logic in separate script from handler

Co-authored-by: Eugene Kozlov <kozlove@dev-dsk-kozlove-2b-2198d727.us-west-2.amazon.com>

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

Labels

contribution/core