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

Fixing register for services with reserved names #777

Merged
merged 3 commits into from
Dec 13, 2022
Merged

Conversation

ryan-cahill
Copy link
Member

@ryan-cahill ryan-cahill commented Dec 13, 2022

Overview

Fixes registering a component with a service that includes a reserved name (https://gitlab.com/architect-io/architect-cli/-/issues/579)

Changes

  • Passing architect_ref to getImage rather than just ref

Tests

  • Added test that registers the superset

@@ -331,8 +331,7 @@ export class DockerComposeUtils {
if (service.build) {
if (!service.image) {
// eslint-disable-next-line unicorn/consistent-destructuring
const image = options.getImage ? options.getImage(node.config.metadata.ref) : node.ref;
service.image = image;
service.image = options.getImage ? options.getImage(node.config.metadata.architect_ref) : node.ref;
Copy link
Member

Choose a reason for hiding this comment

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

To clarify does the config.metadata.ref include the reserved name?

I probably added architect_ref, but was immediately confused by this logic.
I'll merge this, but might be worth adding a tech debt ticket since there are 3 refs right now.
node.ref // Computed ref - reserved_name
node.config.metadata.ref // Feel like this should be the internal ref and remove architect_ref
node.config.metadata.architect_ref

Copy link
Member Author

@ryan-cahill ryan-cahill Dec 13, 2022

Choose a reason for hiding this comment

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

Registering the service stateful-api with the reserved name stateful-reserved-name:

  • node.ref = stateful-reserved-name
  • node.config.metadata.ref = stateful-reserved-name
  • node.config.metadata.architect_ref = superset.services.stateful-api

Where for comparison, stateless-app that doesn't have a reserved name looks like this:

  • node.ref = superset--stateless-app
  • node.config.metadata.ref = superset.services.stateless-app
  • node.config.metadata.architect_ref = superset.services.stateless-app

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed that that's confusing and I'll make a ticket

@tjhiggins tjhiggins merged commit 6f465af into rc Dec 13, 2022
@tjhiggins tjhiggins deleted the fixing-register branch December 13, 2022 14:46
github-actions bot pushed a commit that referenced this pull request Dec 13, 2022
# [1.30.0-rc.3](v1.30.0-rc.2...v1.30.0-rc.3) (2022-12-13)

### Bug Fixes

* **register:** Fixing register for services with reserved names ([#777](#777)) ([6f465af](6f465af))
github-actions bot pushed a commit that referenced this pull request Dec 14, 2022
# [1.30.0](v1.29.0...v1.30.0) (2022-12-14)

### Bug Fixes

* **graph:** Fix dependency edges for new interfaces spec ([#776](#776)) ([1913d8e](1913d8e))
* **register:** Fixing register for services with reserved names ([#777](#777)) ([6f465af](6f465af))
* **reserved_name:** Remove architect_ref to avoid confusion ([#778](#778)) ([4967e7d](4967e7d))
* **sentry:** Log flags/commands as tags for searching ([#762](#762)) ([d821dea](d821dea))
* **webkit:** Add check for regex lookbehind support ([#770](#770)) ([8698a2d](8698a2d))
* **webkit:** regex ([#771](#771)) ([b74cccd](b74cccd))
* **webkit:** Use RegExp so catch triggers ([cccfded](cccfded))

### Features

* **spec:** Deprecate top level interfaces block ([#775](#775)) ([9144120](9144120))
@github-actions
Copy link

🎉 This PR is included in version 1.30.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants