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.

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

Fixing register for services with reserved names #777

Merged
merged 3 commits into from
Dec 13, 2022
Merged

Fixing register for services with reserved names #777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
36731aa
fixing register when a reserved name is used for a service
ryan-cahill Dec 13, 2022
c9fed02
undoing debugging update
ryan-cahill Dec 13, 2022
34ccd96
better reserved name
ryan-cahill Dec 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/common/docker-compose/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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

}

// Optimization to check if multiple services share the same dockerfile/build config and avoid building unnecessarily
Expand Down
38 changes: 37 additions & 1 deletion test/commands/register.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ import { expect } from 'chai';
import fs from 'fs-extra';
import yaml from 'js-yaml';
import path from 'path';
import untildify from 'untildify';
import sinon, { SinonSpy, SinonStub } from 'sinon';
import untildify from 'untildify';
import { ServiceSpec, TaskSpec, validateSpec } from '../../src';
import AccountUtils from '../../src/architect/account/account.utils';
import ComponentRegister from '../../src/commands/register';
Expand Down Expand Up @@ -193,6 +193,42 @@ describe('register', function () {
expect(ctx.stdout).to.contain('Successfully registered component');
});

mockArchitectAuth()
.nock(MOCK_API_HOST, api => api
.get(`/accounts/examples`)
.reply(200, mock_account_response)
)
.nock(MOCK_API_HOST, api => api
.post(/\/accounts\/.*\/components/, (body) => body)
.reply(200)
)
.nock(MOCK_API_HOST, api => api
.get(`/accounts/examples/components/superset/versions/1.0.0`)
.reply(200)
)
.nock(MOCK_REGISTRY_HOST, api => api
.head(`/v2/examples/superset.services.stateless-app/manifests/1.0.0`)
.reply(200)
)
.nock(MOCK_REGISTRY_HOST, api => api
.head(`/v2/examples/superset.services.stateful-api/manifests/1.0.0`)
.reply(200)
)
.nock(MOCK_REGISTRY_HOST, api => api
.head(`/v2/examples/superset.services.stateful-frontend/manifests/1.0.0`)
.reply(200)
)
.nock(MOCK_REGISTRY_HOST, api => api
.head(`/v2/examples/superset.tasks.curler-build/manifests/1.0.0`)
.reply(200)
)
.stdout({ print })
.stderr({ print })
.command(['register', 'test/mocks/superset/architect.yml', '-t', '1.0.0', '-a', 'examples'])
.it('it reports to the user that the superset was registered successfully', ctx => {
expect(ctx.stdout).to.contain('Successfully registered component');
});

mockArchitectAuth()
.nock(MOCK_API_HOST, api => api
.get(`/accounts/examples`)
Expand Down
3 changes: 2 additions & 1 deletion test/mocks/superset/architect.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ services:
protocol: postgresql

stateful-api:
reserved_name: stateful-reserved-name
build:
args:
build_arg_string: arg_value
Expand Down Expand Up @@ -210,7 +211,7 @@ services:
args:
NODE_ENV: development
depends_on:
- stateful-api
- stateful-reserved-name
scaling:
min_replicas: 1
max_replicas: 3
Expand Down