Skip to content
This repository was archived by the owner on Jan 5, 2019. It is now read-only.

Remove task-specific schema from hook.task#77

Merged
djmitche merged 4 commits intotaskcluster:masterfrom
kritisingh1:schema-update
Feb 27, 2018
Merged

Remove task-specific schema from hook.task#77
djmitche merged 4 commits intotaskcluster:masterfrom
kritisingh1:schema-update

Conversation

@kritisingh1
Copy link
Contributor

@kritisingh1 kritisingh1 commented Feb 27, 2018

This is in reference to Bug 1438019 which requires updating the current schemas in accordance with JSON-e so that hook.task has type: object.

@djmitche djmitche self-requested a review February 27, 2018 16:16
Copy link
Contributor

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

I'll post a longer comment about this issue in the bug, but for the moment, a quick note about commits and pull requests. A pull request and commit message should describe what is changing directly, rather than just referring to a bug. The title of this PR, "fixes Bug 1438019", doesn't tell me much (I don't remember what that bug number refers to!).

We have some guidelines in our documentation. Those are pretty common to git-based open source projects, not just Taskcluster, so a good skill to learn.

Copy link
Contributor

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

This looks great!

Two things to add:

Copy link
Contributor

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

I added a little bit more text to the description :)

@djmitche djmitche changed the title fixes Bug 1438019 Remove task-specific schema from hook.task Feb 27, 2018
@djmitche djmitche merged commit 8b50cf5 into taskcluster:master Feb 27, 2018
@kritisingh1
Copy link
Contributor Author

@djmitche the test that calls createTask still needs to be added, right? Thanks :)

@djmitche
Copy link
Contributor

Oh right! Please make a new PR for that. Silly me!!

@kritisingh1
Copy link
Contributor Author

Sure. Thanks a lot!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants