Skip to content

feat(registry-target): Allow batched updates w/ new config - #249

Merged
BYK merged 7 commits into
masterfrom
byk/feat/multi-registry-target
Jun 1, 2021
Merged

BYK merged 7 commits into
masterfrom
byk/feat/multi-registry-target

Conversation

@BYK

@BYK BYK commented May 28, 2021

Copy link
Copy Markdown
Member

Fixes #48 by introducing a new registry target config structure while
keeping backward compatibility.

Now we expect two dicts under the names apps and sdks where the keys
the canonical names and the values are individual configs for these
canonical names.

It also does a single commit for all changes instead of one commit per
canonical name.

This new structure also has the side effect of making it very hard to
miss required configuration.

Tested with craft in dry-run mode.

Fixes #48 by introducing a new registry target config structure while
keeping backward compatibility.

Now we expect two dicts under the names `apps` and `sdks` where the keys
the canonical names and the values are individual configs for these
canonical names.

It also does a single commit for all changes instead of one commit per
canonical name.

This new structure also has the side effect of making it very hard to
miss required configuration.

Tested with `craft` in dry-run mode.
@BYK
BYK requested review from chadwhitacre, jan-auer and tonyo May 28, 2021 06:59
Comment thread src/artifact_providers/base.ts
Comment thread src/artifact_providers/base.ts Outdated
Comment thread src/targets/awsLambdaLayer.ts Outdated
Comment thread src/utils/registry.ts

@chadwhitacre chadwhitacre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A couple questions during my first pass here ...

I see that listArtifactsForRevision occurs in stores/zeus.ts. Looks like that file is copy/pasta, and I see that it can go away at any time (right? and actually I think all of Zeus can go away basically now-ish?). Removing entirely is out of scope for this PR, of course, but just wanted to double check that we don't need to update the logic in the store here. Do we? :)

Secondly, I don't see any README update. Surely with a new config file format we should have a README update here, no?

Comment thread .craft.yml
@BYK

BYK commented Jun 1, 2021

Copy link
Copy Markdown
Member Author

I see that listArtifactsForRevision occurs in stores/zeus.ts. Looks like that file is copy/pasta, and I see that it can go away at any time (right? and actually I think all of Zeus can go away basically now-ish?). Removing entirely is out of scope for this PR, of course, but just wanted to double check that we don't need to update the logic in the store here. Do we? :)

Eh, great catch. I think Zeus stuff predates the BaseArtifactProvider days. I'll do a separate pass to DRY that up. I need to do a full audit to remove Zeus and I don't have enough time for that right now. Will schedule tho.

Secondly, I don't see any README update. Surely with a new config file format we should have a README update here, no?

For sure, great catch thanks!

Comment thread src/targets/registry.ts Outdated
Comment thread src/targets/registry.ts Outdated
Comment thread src/targets/registry.ts
Comment thread src/targets/registry.ts
Comment thread src/targets/registry.ts
Comment thread src/utils/registry.ts
Comment thread src/targets/registry.ts
Comment thread README.md Outdated

@chadwhitacre chadwhitacre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ship it! 馃憤

@BYK
BYK merged commit 03b2a1c into master Jun 1, 2021
@BYK
BYK deleted the byk/feat/multi-registry-target branch June 1, 2021 21:57
BYK added a commit that referenced this pull request Jun 10, 2021
BYK added a commit that referenced this pull request Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

registry target: batch updates if there are multiple packages to release

3 participants