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

Allow dependencies to be defined on params #70

Merged
merged 1 commit into from Jul 6, 2018
Merged

Allow dependencies to be defined on params #70

merged 1 commit into from Jul 6, 2018

Conversation

philipgough
Copy link
Contributor

@philipgough philipgough changed the title Allow dependencies to be defined on params WIP: Allow dependencies to be defined on params May 4, 2018
@dymurray
Copy link
Contributor

I have read through the attached issue and I like the solution that you guys came up with. Since this will involve an addition to the Bundle spec this type of change will require a version bump of the Bundle Spec along with documentation updates (see here for current documentation around Bundle spec).

@dymurray
Copy link
Contributor

From discussing at the community meeting we agreed that this will require a minor version bump of the Bundle spec bringing us to version 1.1 (per our docs since this is the addition of an optional field). Please update developers.md with an example of using this and defining its use in the Bundle spec as I linked above.

Updatable bool `json:"updatable"`
DisplayType string `json:"displayType,omitempty" yaml:"display_type,omitempty"`
DisplayGroup string `json:"displayGroup,omitempty" yaml:"display_group,omitempty"`
Dependencies []Dependency `json:"dependencies,omitempty" yaml:"dependencies,omitempty"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can only handle a single dependency here currently in the broker due to limitations in the frontend lib but I think to future proof where we may be able to do this if we can extend that lib, leaving this as a slice makes sense IMO. What do people think?

The spec itself should be able to support multiple dependencies if my understanding of it is correct.

cc @david-martin

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed @philipgough allow multiple dependencies.

@dymurray
Copy link
Contributor

dymurray commented Jun 1, 2018

Thanks for doing a doc PR!

Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

I'm actually okay with this, I don't see any issues.

@@ -1,3 +1,6 @@
vendor*
coverage-all.out
coverage.out

# IDE baggage
.idea/
Copy link
Contributor

Choose a reason for hiding this comment

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

this is acceptable

Updatable bool `json:"updatable"`
DisplayType string `json:"displayType,omitempty" yaml:"display_type,omitempty"`
DisplayGroup string `json:"displayGroup,omitempty" yaml:"display_group,omitempty"`
Dependencies []Dependency `json:"dependencies,omitempty" yaml:"dependencies,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed @philipgough allow multiple dependencies.

@philipgough philipgough changed the title WIP: Allow dependencies to be defined on params Allow dependencies to be defined on params Jun 26, 2018
@dymurray dymurray merged commit 50703ff into automationbroker:master Jul 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants