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 goals to receive implementations and listeners #504

Merged
merged 17 commits into from
Sep 2, 2018

Conversation

cdupuis
Copy link
Member

@cdupuis cdupuis commented Sep 2, 2018

This PR brings infrastructure to create Typed Goals: goals that carry their own ExecuteGoal implementation. See Autofix for an example.

The goal will find the SDM instances and register the goal implementations with it, so that from a user API all is left is to do:

whenPushSatisfies(AnyPush).setGoals(new Autofix("my-autofix").with(AddThirdPartyLicense));

This will allow us to provide a rich ecosystem of Goals for different types of delivery concerns without the requirement to change the core SoftwareDeliveryMachine API.

The main changes in this PR are backwards compatible and only additive. Only exception is the move of the well-known-goals to sdm.

@cdupuis cdupuis added changelog:added Add this issue or pull request to added changelog section auto-merge:on-approve Auto-merge on review approvals auto-merge-method:squash Auto-merge with squash and merge labels Sep 2, 2018
@cdupuis cdupuis changed the title Fix approvalRequired message not being used Allow goals to receive implementations and listeners Sep 2, 2018
const matchedNames = this.implementations.filter(m =>
m.implementationName === goal.fulfillment.name &&
m.goal.context === goal.externalKey);

const matchedGoalImplementations = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

does this mean there are no more push tests on implementations? ... or that they aren't used?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, they are being used and evaluated when we SetOnGoalsOnPush. I added this here again last week which was a mistake.

const {goalSet, goal, goalSetId, state, id, providerId, url} = parameters;
const fulfillment = parameters.fulfillment || {method: "other", name: "unspecified"};
const { goalSet, goal, goalSetId, state, id, providerId, url } = parameters;
const fulfillment = parameters.fulfillment || { method: SdmGoalFulfillmentMethod.Other, name: "unkown" };
Copy link
Contributor

Choose a reason for hiding this comment

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

spelling: unkown

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point.

@@ -268,6 +275,8 @@ export abstract class AbstractSoftwareDeliveryMachine<O extends SoftwareDelivery
goalSetters: Array<GoalSetter | GoalSetter[]>) {
super();
// If we didn't get any goal setters don't register a mapping
registrableManager().register(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

what does the global __registrable do?

Copy link
Contributor

Choose a reason for hiding this comment

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

it should be registerable

Copy link
Contributor

@johnsonr johnsonr left a comment

Choose a reason for hiding this comment

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

This is a significant improvement.

@@ -268,6 +275,8 @@ export abstract class AbstractSoftwareDeliveryMachine<O extends SoftwareDelivery
goalSetters: Array<GoalSetter | GoalSetter[]>) {
super();
// If we didn't get any goal setters don't register a mapping
registrableManager().register(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

it should be registerable

@atomist-bot atomist-bot merged commit 707bb8f into master Sep 2, 2018
@atomist-bot
Copy link
Contributor

Pull request auto merged by Atomist.

  • 1 approved review by @johnsonr
  • 2 successful checks

[atomist:generated] [auto-merge:on-approve]

@atomist-bot atomist-bot deleted the richer-goals branch September 2, 2018 22:45
atomist-bot added a commit that referenced this pull request Sep 2, 2018
[atomist:generated]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge:on-approve Auto-merge on review approvals auto-merge-method:squash Auto-merge with squash and merge changelog:added Add this issue or pull request to added changelog section
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants