feat(deploy-gen): adds new main deploy generator #2891
Conversation
cianmSAP
commented
Feb 7, 2025
•
cianmSAP
commented
- Add new deployment sub generator
- Tests to be added
🦋 Changeset detectedLatest commit: f9f188f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
| @@ -0,0 +1,3 @@ | |||
| // When deployment generator is bundled the namespacing is relative to the root generator | |||
| export const bundledRootGeneratorName = '@sap/fiori:fiori-deployment'; | |||
There was a problem hiding this comment.
Should this be passed in from @sap/generator-fiori?
| @@ -0,0 +1,467 @@ | |||
| import { basename, dirname, join } from 'path'; | |||
There was a problem hiding this comment.
Do you think we can we reduce the size of this file perhaps by only including direct lifecycle methods. Refactor the private function out to a new file.
| super(args, opts); | ||
| // this.env.adapter.promptModule is undefined when running in YUI | ||
| this.env.adapter.promptModule?.registerPrompt('autocomplete', autocomplete); | ||
| this.launchDeployConfigAsSubGenerator = opts.launchDeployConfigAsSubGenerator ?? false; |
There was a problem hiding this comment.
Do we still need this concept launchDeployConfigAsSubGenerator? If we do then can we determine if there is a caller (composeWith(subgenName')) programmatically instead of expecting the consumer to set this? Might be a stretch...
| * The main deployment configuration generator. | ||
| */ | ||
| export default class extends DeploymentGenerator { | ||
| private readonly launchDeployConfigAsSubGenerator: boolean; |
There was a problem hiding this comment.
Can we document all of these properties as its not clear why they are needed? Should there be instead a deploy 'state' type defined that encapsulates the required state for deploy config generation. This might make for a cleaner implementation.
| export default class extends DeploymentGenerator { | ||
| private readonly launchDeployConfigAsSubGenerator: boolean; | ||
| private readonly launchStandaloneFromYui: boolean; | ||
| private readonly abapChoice: Target = { name: TargetName.ABAP, description: 'ABAP' }; |
There was a problem hiding this comment.
Is it correct to define inquirer related concepts in a generator constructor. Can these be moved to where they are used?
| // Load .env file for api hub config | ||
| dotenv.config(); | ||
|
|
||
| // Application Modeler launches Deployment Configuration Generator YUI. |
There was a problem hiding this comment.
This refers to a non-open source module here (Application Modeler)? Maybe consider a more generic approach that supports 2 cases: 1) Where this is called as a subgenerator 2) Where its called standalone.
| constructor(args: string | string[], opts: DeployConfigOptions) { | ||
| super(args, opts); | ||
| // this.env.adapter.promptModule is undefined when running in YUI | ||
| this.env.adapter.promptModule?.registerPrompt('autocomplete', autocomplete); |
There was a problem hiding this comment.
Im not sure this is a valid comment (deploy already uses this plugin in YUI for transport type ahead I think?). Better if the adaptor is passed down to the inquirers (it is a prompting plugin) and registered there. Inquirers may need to register other plugins...seems to be an assumption here that autocomplete is needed?
| /** | ||
| * The connected system from the generator | ||
| */ | ||
| connectedSystem?: { |
There was a problem hiding this comment.
Might be better to use the existing type since thats where this definition originates from:
import { type OdataServiceAnswers } from '@sap-ux/odata-service-inquirer';
...
connectedSystem?: OdataServiceAnswers['connectedSystem'];
...
| /** | ||
| * Whether the system is a cloud system | ||
| */ | ||
| scp?: boolean; |
There was a problem hiding this comment.
This property is no longer being passed and should be considered deprecated. It can be calculated internally if required?
| } | ||
|
|
||
| public async initializing(): Promise<void> { | ||
| await super.initializing(); |
There was a problem hiding this comment.
Should we also be initialising the VSCode logging here if this generator can be directly launched in YUI...as is done here:
or is that delegated to subgens...in in which case there wont be any vscode logging output from here?
