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

Adds a secondary form to project:create for template, catalog, and initialize options #831

Open
wants to merge 17 commits into
base: development
Choose a base branch
from

Conversation

shawnawsu
Copy link

A new form will be built and served if the --template flag is passed. The fields are template_url which allows for the passing of a url, catalog_url which will list all options from the catalog, and initialize which will initialize the project after creation if selected.

src/Command/Project/ProjectCreateCommand.php Outdated Show resolved Hide resolved
src/Command/Project/ProjectCreateCommand.php Outdated Show resolved Hide resolved
src/Command/Project/ProjectCreateCommand.php Outdated Show resolved Hide resolved
src/Command/Project/ProjectCreateCommand.php Outdated Show resolved Hide resolved
src/Command/Project/ProjectCreateCommand.php Outdated Show resolved Hide resolved
src/Command/Project/ProjectCreateCommand.php Outdated Show resolved Hide resolved
src/Command/Project/ProjectCreateCommand.php Outdated Show resolved Hide resolved
src/Command/Project/ProjectCreateCommand.php Outdated Show resolved Hide resolved
src/Command/Project/ProjectCreateCommand.php Outdated Show resolved Hide resolved
src/Command/Project/ProjectCreateCommand.php Show resolved Hide resolved
config.yaml Outdated Show resolved Hide resolved
@Crell
Copy link
Contributor

Crell commented Sep 4, 2019

If I'm reading this properly, the --template flag serves double duty: Either provide a template (in which case it's a short-circuit way to also run initialize) or provide a catalog to pick from that will interactively ask which project you want. Am I reading it correctly?

If so, then IMO it should be split. Have a --template that specifies a Git URL, which then short-circuits initialize, and have a separate --catalog which specifies a catalog definition that will interactively ask which template you want. But those behaviors should not be folded into a single switch. They're separate activities.

@Kazanir
Copy link
Contributor

Kazanir commented Sep 15, 2019

If I'm reading this properly, the --template flag serves double duty: Either provide a template (in which case it's a short-circuit way to also run initialize) or provide a catalog to pick from that will interactively ask which project you want. Am I reading it correctly?

If so, then IMO it should be split. Have a --template that specifies a Git URL, which then short-circuits initialize, and have a separate --catalog which specifies a catalog definition that will interactively ask which template you want. But those behaviors should not be folded into a single switch. They're separate activities.

We're skipping the "catalog definition" step, that's provided by Accounts based on any of your vendor/organization[NYI]/user-level catalog files, or falling back to the default if none of those are found. The --template key here (if I understand the MR right) will take you into the catalog process if you supply it without an argument for a specific template file, but the catalog itself is being provided by who you are via Accounts, not the CLI flag.

Console is going to support a ?catalog= parameter for the sake of "Try Our Stuff On Platform" buttons but there's really no equivalent CLI functionality. Scripting the ?template input to a specific template file is a pretty likely use case though.

$regions[$region->id] = $region->label;
// First look to the setup/options API for a region list.
$account = $this->api()->getMyAccount(true);
$setupOptions = $this->api()->getClient()->getSetupOptions(NULL, NULL, NULL, $account['username'], NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't the setup options API supposed to fall back to all the available regions automatically, thus making the subsequent $this->api()->getClient()->getRegions() unnecessary?

}

if (empty($catalog)) {
// Should we throw an error here? Do we want to kill the process?
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the getTemplateFields() receives an empty catalog array? If nothing significant -- I'd remove this condition. If something unexpected -- I'd throw an Exception and bail out.

I don't have a clue what am I writing about, right here, so please be free to ignore me if I make no sense at all.

Copy link
Author

Choose a reason for hiding this comment

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

If $catalog comes back as empty it will cause a logic exception since it's an asChoice field. So, do we want it to fail and skip to the next step and just now allow a template choice or stop the process? I'm not sure what the preference is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I'd personally fail it. What do you think @pjcdawkins?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd return [] here, and let Symfony throw the LogicException

if ($runtime) {
$catalog = [];
$catalog_items = $this->api()->getClient()->getCatalog()->getData();
if (!empty($catalog_items)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the if isn't needed - if it's always an array, the foreach won't mind if it's empty

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.

5 participants