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

feat(cli): discover additional app-config yamls #2484

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(cli): discover additional app-config yamls #2484

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 5 additions & 10 deletions packages/cli/src/commands/package-dynamic-plugins/command.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,21 +293,16 @@ function discoverPluginConfigurationFile(
): string | undefined {
// Possible file names, the first match will be used
const supportedFilenames = [
'app-config.dynamic.example.yaml',
'app-config.dynamic.yaml',
'app-config.janus-idp.yaml',
'app-config.backstage-community.yaml',
'app-config.example.yaml',
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about looking for app-config.example.yaml and even the existing app-config.yaml entry that was originally here. One thing I'd like to avoid is accidentally picking up one of these files that may contain a complete backstage configuration file for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can remove app-config.yaml, but please be aware that some plugins uses this currently.

But since this feature is not documented (or?), I'm do what you prefer.

Should I remove it? :)

Copy link
Member

Choose a reason for hiding this comment

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

Yep, we haven't documented this convention at all yet, so we can decide what we want to keep in the longer term. I'm not sure of the origin of even the app-config.janus-idp.yaml convention, either the intent was it's a file that is meant to be handled by the janus-idp CLI or maybe the intent was just to match it with the organization name, like what was done when plugins were moved to the community repo and this app-config.backstage-community.yaml convention was introduced.

That being said, the more I think about it, the more I think maybe it's better to just keep it simple and maybe we should eventually just narrow down this list to app-config.yaml despite my worry that an entire yaml config gets picked up by the tooling, it's much easier to remember :-)

Copy link
Member

Choose a reason for hiding this comment

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

or better still as @davidfestal suggested app-config.dynamic.yaml as the one we document and favor over the other fallbacks, I think that naming makes it pretty clear it's not like the other config files.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe this log statement could be updated as part of this too to mention that.

'app-config.yaml',
];
return supportedFilenames
.map<boolean>((fileName: string) => {
const candidate = path.join(directory, fileName);
return fs.existsSync(candidate);
})
.reduce<string | undefined>((val, current, index) => {
if (typeof val === 'undefined' && current) {
return path.join(directory, supportedFilenames[index]);
}
return val;
}, undefined);
.map(fileName => path.join(directory, fileName))
.find(candidate => fs.existsSync(candidate));
Copy link
Member

Choose a reason for hiding this comment

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

👍 to this change anyways 😄

}

/**
Expand Down
Loading