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

fix: change order of merging configs #33

Merged
merged 3 commits into from
Jan 10, 2024
Merged

fix: change order of merging configs #33

merged 3 commits into from
Jan 10, 2024

Conversation

djamaile
Copy link
Collaborator

baseUrl doesn't get replaced:

❯ yarn backstage-cli config:print --config app-config.deployment.yaml --config app-config.yaml --frontend
yarn run v1.22.19
$ /Users/djamailer/Desktop/projects/backstage-changeset/node_modules/.bin/backstage-cli config:print --config app-config.deployment.yaml --config app-config.yaml --frontend
Loaded config from app-config.deployment.yaml, app-config.yaml
app:
  baseUrl: http://localhost:3000
  title: Scaffolded Backstage App
backend:
  baseUrl: http://localhost:7007
organization:
  name: My Company
integrations:
  github:
    - host: github.com
techdocs:
  builder: local
catalog:
  import:
    entityFilename: catalog-info.yaml
    pullRequestBranchName: backstage-integration

✨  Done in 1.02s.

baseUrl gets properly replaced:

❯ yarn backstage-cli config:print --config app-config.yaml --config app-config.deployment.yaml --frontend
yarn run v1.22.19
$ /Users/djamailer/Desktop/projects/backstage-changeset/node_modules/.bin/backstage-cli config:print --config app-config.yaml --config app-config.deployment.yaml --frontend
Loaded config from app-config.yaml, app-config.deployment.yaml
app:
  title: Scaffolded Backstage App
  baseUrl: http://test
organization:
  name: My Company
backend:
  baseUrl: http://test
integrations:
  github:
    - host: github.com
techdocs:
  builder: local
catalog:
  import:
    entityFilename: catalog-info.yaml
    pullRequestBranchName: backstage-integration

✨  Done in 1.22s.

@djamaile djamaile requested a review from a team as a code owner January 10, 2024 08:59
Signed-off-by: djamaile <rdjamaile@gmail.com>
Copy link
Contributor

@mitchhentgesspotify mitchhentgesspotify left a comment

Choose a reason for hiding this comment

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

This seems purely style and like a no-op, I'm surprised that it's changing behaviour - isn't the callback the first parameter either way? Weird.

Comment on lines 188 to 189
const CONTAINER_SERVICE_URL = containerService.url.apply(url =>
url.endsWith('/') ? url.slice(0, -1) : url
url.endsWith('/') ? url.slice(0, -1) : url,
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're here, do you mind making CONTAINER_SERVICE_URL -> containerServiceUrl? I didn't want to bother a contributor with this nit, but I'm happy to bug you with it 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 0942039

Signed-off-by: djamaile <rdjamaile@gmail.com>
@@ -37,4 +37,4 @@ RUN echo "techdocs:" >> app-config.deployment.yaml
RUN echo " generator:" >> app-config.deployment.yaml
RUN echo " runIn: 'local'" >> app-config.deployment.yaml

CMD ["node", "packages/backend", "--config", "app-config.deployment.yaml", "--config", "app-config.yaml"]
Copy link
Contributor

Choose a reason for hiding this comment

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

What in tarnation is the change here? Weird why this is showing up in the diff.

@djamaile djamaile merged commit 2e04292 into main Jan 10, 2024
1 check passed
@djamaile djamaile deleted the djamaile-patch-1 branch January 10, 2024 10:52
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.

2 participants