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

catalog saver to support 'application', 'template', and 'entity' #109

Merged
merged 4 commits into from Nov 20, 2018

Conversation

Projects
None yet
4 participants
@ahgittin
Copy link
Contributor

ahgittin commented Nov 16, 2018

in line with apache/brooklyn-server#1015
also when using a template the metadata is not populated as the intention is probably not to overwrite,
unless the user selects to save it as a template

ahgittin added some commits Nov 16, 2018

catalog saver to support 'application', 'template', and 'entity'
in line with apache/brooklyn-server#1015
also when using a template the metadata is not populated as the intention is probably not to overwrite,
unless the user selects to save it as a template
@tbouron
Copy link
Member

tbouron left a comment

Thanks @ahgittin, that looks good.

However, I'm not sure about the label + description you gave to the different type of blueprint you can choose from:

  • Application entity is the previous entity type I presume?
  • Application template is the previous template type I presume?
  • What is then Extended entity?

The tooltip don't help either, the differences are too subtile IMO. I believe this would not make a lot of sense to our users, especially the newly introduced Extended entity.

Could we find a way to simply this further?

.directive('catalogVersion', ['$parse', catalogVersionDirective])
.directive('blueprintNameOrSymbolicNameAndBundleIdRequired', blueprintNameOrSymbolicNameAndBundleIdRequiredDirective)

This comment has been minimized.

Copy link
@tbouron

tbouron Nov 20, 2018

Member

Wow, that a very loooooong name. Could we come up with something shorter like blueprintNameValidator? The name doesn't need to say what it does exactly

This comment has been minimized.

Copy link
@ahgittin

ahgittin Nov 20, 2018

Author Contributor

okay, compromise, changed to composerBlueprintNameValidator

This comment has been minimized.

Copy link
@tbouron

tbouron Nov 20, 2018

Member

Sounds good 👍

@@ -105,16 +105,31 @@ export function MainController($scope, $element, $log, $state, $stateParams, brB

vm.saveToCatalogConfig = {};
if (edit) {
console.log("edit", edit);

This comment has been minimized.

Copy link
@tbouron

tbouron Nov 20, 2018

Member

To remove

This comment has been minimized.

Copy link
@ahgittin

ahgittin Nov 20, 2018

Author Contributor

good spot

@ahgittin

This comment has been minimized.

Copy link
Contributor Author

ahgittin commented Nov 20, 2018

@tbouron agree it would be nice to make the differences clearer but not sure how. the test to apply is whether it's clearer than before where we prompted for "Application" or "Entity" with no difference, and I think it is. i've expanded the wording slightly, and welcome further enhancements, but anything more than quick fixes I think will be a longer discussion and shouldn't block this:

  • Application entity: Save as an application entity which can be deployed on its own, or configured and used in blueprints but only config and sensors declared at the root are accessible in the Composer ('application' item type)
  • Application template: Save as a blueprint template which can be used as an editable starting point for blueprints or used as an application entity ('template' item type)
  • Extended entity: Save as an entity which can be configured and used in blueprints, exposing the config and adjuncts it inherits ('entity' item type)
@ahgittin

This comment has been minimized.

Copy link
Contributor Author

ahgittin commented Nov 20, 2018

one other change, we weren't watching bundle and symbolicName so the validator didn't do everything it should, that's fixed too now

@aledsage

This comment has been minimized.

Copy link
Contributor

aledsage commented Nov 20, 2018

@ahgittin for 'template', is that really what it means in the product from a user's perspective? Something declared as a 'template' will appear in the quick launch, whereas other things will not. When you click on it in the 'quick launch', then a dialog pops up with two buttons: deploy and open in editor. The latter opens a simple text editor in the quick-launch dialog, rather than re-directing to the blueprint composer.

Your description of 'template' is accurate for when this was originally added to the code base, but I don't think that is what it does now, or how it is used now. That description won't make sense to a user, based on how the Brooklyn UI behaves.

@tbouron

This comment has been minimized.

Copy link
Member

tbouron commented Nov 20, 2018

@ahgittin I'm wondering if the tooltip texts should include the item type, the same way you describe them in your previous comment. I think it's a good compromise so users have a reference they can scan for.

@ahgittin

This comment has been minimized.

Copy link
Contributor Author

ahgittin commented Nov 20, 2018

@tbouron the itemType is included in the tooltip text, in parentheses at the end

@aledsage i think you're flagging a problem with quick launch that it is cheating a bit using the catalog_template tag for what should be a special quick_launch or favourites mechanism. that should be fixed as a separate task. but until we've done this i'll add to that tooltip text something like that "In some contexts this prioritizes the blueprint for inclusion in quick-selection views."

@tbouron

This comment has been minimized.

Copy link
Member

tbouron commented Nov 20, 2018

@aledsage Are you happy to address your comment in a subsequent PR? I'm ok to merge this one but would like to check with you first.

@aledsage

This comment has been minimized.

Copy link
Contributor

aledsage commented Nov 20, 2018

Sure, happy to merge and we can make subsequent improvements in other PRs. Merging now.

@asfgit asfgit merged commit 959ea3a into apache:master Nov 20, 2018

1 check passed

Jenkins: brooklyn-ui-pull-request SUCCESS 10 tests run, 0 skipped, 0 failed.
Details

asfgit pushed a commit that referenced this pull request Nov 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.