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

feat(cli): allow overriding imagery names #2169

Merged
merged 10 commits into from
May 16, 2022

Conversation

blacha
Copy link
Member

@blacha blacha commented Apr 29, 2022

No description provided.

@blacha blacha requested a review from a team as a code owner April 29, 2022 02:12
@lgtm-com
Copy link

lgtm-com bot commented Apr 29, 2022

This pull request introduces 1 alert when merging 1375a3c into 4ad2d37 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@blacha blacha force-pushed the feat/allow-custom-imagery-names branch from 1375a3c to fe6e2c0 Compare May 11, 2022 02:22
@Wentao-Kuang
Copy link
Contributor

Build failure.

@@ -26,15 +28,24 @@ export async function Import(req: LambdaHttpRequest): Promise<LambdaHttpResponse
targetTms = tileMatrix;
}

// Find the imagery from s3
// Validate the s3 path
Copy link
Contributor

Choose a reason for hiding this comment

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

@blacha I added some more here, to validate the imagery name before pass it to the CogJobContext.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is going to give very weird imagery names, maybe we should just use :ulid_:year to show its a completly made up name?

Some of these import paths will be completely meaningless, imagine a folder s3://a/2022/04/eca/RGB/ what sort of "name" would we get from this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, we cannot just add uild_year into CogJobContext. This will make a different hash each time from CogJobContext.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am going to make this imageryName as optional attributions for CogJobContext. And add this after creating hash.

Wentao-Kuang
Wentao-Kuang previously approved these changes May 16, 2022
@Wentao-Kuang Wentao-Kuang dismissed their stale review May 16, 2022 03:15

Need changes.

@@ -22,7 +22,9 @@ export const CogJobFactory = {
*/
async create(ctx: JobCreationContext): Promise<CogJob> {
const id = ctx.override?.id ?? ulid.ulid();
const imageryName = basename(ctx.sourceLocation.path).replace(/\./g, '-'); // batch does not allow '.' in names
let imageryName = ctx.imageryName;
if (imageryName == null) imageryName = basename(ctx.sourceLocation.path).replace(/\./g, '-'); // batch does not allow '.' in names
Copy link
Member Author

Choose a reason for hiding this comment

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

this was removed, we dont use the imagery name inside of batch so we can remove the .replace

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed. Thanks.

@@ -96,6 +96,7 @@ export class ActionCogCreate extends CommandLineAction {
// Output file exists don't try and overwrite it
if (outputExists) {
logger.warn({ targetPath }, 'CogCreate:OutputExists');
await this.checkJobStatus(job, logger);
Copy link
Contributor

Choose a reason for hiding this comment

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

@blacha I have added this check in here. In cases, partially processed file list never get to CogCreate:JobComplete to create 'ts_' and 'im_' records.

@kodiakhq kodiakhq bot merged commit 5c3bdd8 into master May 16, 2022
@kodiakhq kodiakhq bot deleted the feat/allow-custom-imagery-names branch May 16, 2022 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants