Add Bindings view in Hooks create page #730

arshadkazmi42 · 2019-05-12T18:40:32Z

Fixes #610

Attaching snapshot of the page after changes

helfi92 · 2019-05-13T14:09:39Z

ui/src/components/HookForm/index.jsx

+                </IconButton>
+              </ListItem>
+            ))}
+          </List>


Now that we have bindings, we should add a "Schedule" subheader below it and remove the label="Schedule" prop from the TextField component.

helfi92 · 2019-05-13T14:19:26Z

ui/src/components/HookForm/index.jsx

+                    />
+                  }
+                />
+              </ListItem>


There seems to be a lot of duplicated logic with localhost:5080/pulse-messages. I was thinking maybe we could create a PulseBindings component that will take care of creating

component/HookForm and views/PulseMessages would then use that component and feed the appropriate props. Maybe something like:

<PulseBindings bindings={...} onBindingAdd={this.handleBindingAdd} onBindingRemove={this.handleBindingRemove} ... />

What do you think?

Actually, I was thinking the same, I was thinking of using existing PulseMessages but that does not seem to help.
I didn't think of creating a new component.
This sounds good. Let me work on it.

Awesome, thanks @arshadkazmi42.

helfi92

Looks really good overall :)

helfi92 · 2019-05-15T20:40:11Z

ui/src/components/PulseBindings/index.jsx

+              <ListSubheader className={classes.subheader}>
+                Bindings
+              </ListSubheader>
+            }>


"Bindings" and "Schedule" don't seem to have the same styling. "Bindings" seems to be smaller. Maybe we can avoid putting the title inside the component and just have it defined in the hook form view.

Actually issue was with adding the bidings inside, subheader class was missing from PulseBinding component, due to that is was not rendering with correct styles.

I have added subheader style in the component and now it looks fine.

helfi92 · 2019-05-15T20:40:46Z

ui/src/components/PulseBindings/index.jsx

+          <List
+            className={classes.inputList}
+            subheader={
+              <ListSubheader className={classes.subheader}>


subheader doesn't seem to be used.

This was related to the above comment.

helfi92 · 2019-05-15T20:42:05Z

ui/src/components/PulseBindings/index.jsx

+      onBindingAdd,
+      onBindingRemove,
+      onChange,
+    } = this.props;


Let's add static propTypes. We can also add static defaultProps if certain props are not required.

I feel all the props are required for this.
I have written static propTypes something like this.

static propTypes = { pulseExchange: string.isRequired, pattern: string.isRequired, bindings: arrayOf(object).isRequired, onBindingAdd: func.isRequired, onBindingRemove: func.isRequired, onChange: func.isRequired, };

Also, I am not sure if we would not need static defaultProps in this.

Let me know if there is anything missing

Looks good.

helfi92

STR:

Navigate to http://localhost:5080/hooks/garbage/abc456 and login
Add a dummy binding and click on "+"

Actual: The save button is not clickable. The form is not aware of the binding.
Expected: The save button becomes clickable. Clicking on the save icon and then refreshing the page should show the saved binding.

arshadkazmi42 · 2019-05-17T16:04:41Z

@helfi92
I have made changes to pass bindings with payload, I don't have backend setup will you be able to verify it?
Or can you direct me to backend setup guide

helfi92 · 2019-05-21T13:51:03Z

I have made changes to pass bindings with payload, I don't have backend setup will you be able to verify it?
Or can you direct me to backend setup guide

Sure thing. You can look at:

Let me know if you have any questions.

arshadkazmi42 · 2019-05-22T15:09:46Z

@helfi92 I got the backend setup.
I guess bindngs needs to be passed using the graphql mutation

mutation UpdateHook($hookGroupId: ID!, $hookId: ID!, $payload: HookInput!) {
  updateHook(hookGroupId: $hookGroupId, hookId: $hookId, payload: $payload) {
    hookId
  }
}

I am not sure what is the key I have to use to pass binding.
Can you help me out with this, how binding needs to be passed in this

helfi92 · 2019-05-22T21:30:09Z

Good question! I think the bindings is part of the payload. The data structure for a hook is defined in https://github.com/taskcluster/taskcluster/blob/87facc8e7fc4580440bf730e2a0f5e080aba3154/services/hooks/schemas/v1/hook-definition.yml. You will probably just need to do:

diff --git a/services/web-server/src/graphql/Hooks.graphql b/services/web-server/src/graphql/Hooks.graphql
index 92f378b8a..808cd6f52 100644
--- a/services/web-server/src/graphql/Hooks.graphql
+++ b/services/web-server/src/graphql/Hooks.graphql
@@ -72,11 +72,17 @@ type DeleteHook {
   hookId: ID
 }
 
+type HookBinding {
+  exchange: String!
+  routingKeyPattern: String!
+}
+
 input HookInput {
   metadata: HookMetadataInput!
   schedule: [String]
   expires: String
   deadline: String
+  bindings: [HookBinding]
   task: JSON!
   triggerSchema: JSON
 }

Hope that helps.

helfi92 · 2019-05-23T16:04:38Z

services/web-server/src/graphql/Hooks.graphql

@@ -72,11 +72,17 @@ type DeleteHook {
  hookId: ID
 }

+input HookBinding {


Does this work for you? I think it should be type, not input.

On adding type web-server was throwing error and changing it to type was suggested. So I changed it to input

You are correct. My mistake :)

helfi92 · 2019-05-23T17:56:00Z

ui/src/components/HookForm/index.jsx

@@ -182,7 +183,7 @@ export default class HookForm extends Component {
  state = {
    hook: null,
    hookLastFires: null,
-    pattern: '#',
+    routingKeyPattern: '#',


The server expects this property name to be pattern. I don't think this will work atm.

It turns out the hook service expects it to be routingKeyPattern. Sorry for that. Now that you have the server setup, are you able to add a hook to http://localhost:5080/hooks/garbage/abc456 and see if it saves successfully?

When trying to save i was getting 401 Authentication failed.
I thought it needs some level of access to save it. So couldn't validate it

How were you signing in? I can provide you with the required level of access.

You will need to use the manual siginin ./taskcluster signin --name taskcluster-web-server. The taskcluster binary can be downloaded from https://github.com/taskcluster/taskcluster-cli#installation. The taskcluster signin command will open a browser where you will be able to login then upon logging in, the binary will output the credentials in the terminal. Those credentials is what you need to login.

I am doing this, in browser I used auth0 login and used those credentials to login in UI

What is the exact error you are seeing when you try to create/update a hook?

Do you get the same error when creating/updating a hook via https://tools.taskcluster.net/?

arshadkazmi42 · 2019-05-24T14:26:36Z

What is the exact error you are seeing when you try to create/update a hook?

arshadkazmi42 · 2019-05-24T14:29:14Z

Looks like I was using old clientID. This is the error I am getting after using fresh clientID

arshadkazmi42 · 2019-05-24T14:31:41Z

Do you get the same error when creating/updating a hook via https://tools.taskcluster.net/?

This is the error I am getting on this website create

helfi92 · 2019-05-24T14:32:09Z

That looks better. The error is about insufficient scopes. What are you missing for editing http://localhost:5080/hooks/garbage/arshad? Can you post a screenshot of the error you are seeing in the UI?

arshadkazmi42 · 2019-05-24T14:34:58Z

That looks better. The error is about insufficient scopes. What are you missing for editing http://localhost:5080/hooks/garbage/arshad? Can you post a screenshot of the error you are seeing in the UI?

helfi92 · 2019-05-24T14:36:30Z

OK, can you try again. I've given you permissions to play with the garbage/arshad hook.

arshadkazmi42 · 2019-05-24T15:03:37Z

Got it working now. Its saving.
But not getting the saved bindings back in response, I think select graphql query needs to be changed. Can you tell me where exactly is the select query for bindngs?

helfi92 · 2019-05-24T15:09:53Z

services/web-server/src/graphql/Hooks.graphql

@@ -72,11 +72,17 @@ type DeleteHook {
  hookId: ID
 }

+input HookBinding {
+  exchange: String!
+  pattern: String!


I think this property needs to be renamed to be in line with what the hook client expects.

The actual mutations are happening in services/web-server/src/resolvers/hooks.js.

Defintion of a hook: https://github.com/taskcluster/taskcluster/blob/87facc8e7fc4580440bf730e2a0f5e080aba3154/services/hooks/schemas/v1/hook-definition.yml

Definition of a binding: https://github.com/taskcluster/taskcluster/blob/87facc8e7fc4580440bf730e2a0f5e080aba3154/services/hooks/schemas/v1/bindings.yml

I think this property needs to be renamed to be in line with what the hook client expects.

I have updated this

Let me checkin my latest changes

After saving, reloading the page should display the existent bindings that were saved earlier.

On page load this query is getting fired

"query Hook($hookGroupId: ID!, $hookId: ID!) { hook(hookGroupId: $hookGroupId, hookId: $hookId) { hookGroupId hookId schedule metadata { name description owner emailOnError __typename } status { nextScheduledDate lastFire { ... on HookSuccessfulFire { time taskId __typename } ... on HookFailedFire { time error __typename } ... on NoFire { result __typename } __typename } __typename } triggerSchema task __typename } hookLastFires(hookGroupId: $hookGroupId, hookId: $hookId) { taskId firedBy taskCreateTime result error __typename } } "

And I don't see bindings field getting queried in this.
I tried adding bindings in this query but its giving 404 after adding bindings in the above query

Ah, you would need to add it in web-server:

diff --cc ui/src/views/PulseMessages/index.jsx index 85010860d,4371e89d6..000000000 --- a/ui/src/views/PulseMessages/index.jsx +++ b/ui/src/views/PulseMessages/index.jsx diff --git a/services/web-server/src/graphql/Hooks.graphql b/services/web-server/src/graphql/Hooks.graphql index 1c59765f3..040ca0f52 100644 --- a/services/web-server/src/graphql/Hooks.graphql +++ b/services/web-server/src/graphql/Hooks.graphql @@ -51,6 +51,7 @@ type HookStatus { type Hook { hookGroupId: ID! hookId: ID! + bindings: [HookBinding] metadata: HookMetadata! schedule: [String]! task: JSON! @@ -72,7 +73,12 @@ type DeleteHook { hookId: ID } -input HookBinding { +type HookBinding { + exchange: String! + routingKeyPattern: String! +} + +input HookBindingInput { exchange: String! routingKeyPattern: String! } @@ -82,7 +88,7 @@ input HookInput { schedule: [String] expires: String deadline: String - bindings: [HookBinding] + bindings: [HookBindingInput] task: JSON! triggerSchema: JSON }

I am getting insufficient scopes error again

{"errors":[{"message":"Client ID mozilla-auth0/oauth2|firefoxaccounts|967134a2ed244f61b75935fc10b9119c|arshadkazmi42@gmail.com/taskcluster-web-server does not have sufficient scopes and is missing the following scopes:\n\n```\n{\n \"AllOf\": [\n \"hooks:modify-hook:garbage/arshad\",\n \"assume:hook-id:garbage/arshad\"\n ]\n}\n```\n\n

can you add access for this client id to add bindings? Or can you let me know how can I do this, if my client id changes

helfi92 · 2019-05-29T13:09:57Z

ui/src/components/PulseBindings/index.jsx

@@ -50,6 +50,7 @@ import DeleteIcon from 'mdi-react/DeleteIcon';
 }))
 export default class PulseBindings extends Component {
  static propTypes = {
+    patternName: string,


Why do we need a patternName prop?

in PulseMessages component, for bindings pattern key is used so I am passing patternName as input to component, if there is no name passed it will default it to pattern.
As on addBinding function call, uses this name as key and same key is used while rendering

@helfi92 Can you also look at this comment, I don't want this to get lost in our discussions.

I was planning to look at this once the rebase is done. Right now there's some merge conflict in this file so I didn't bother looking at it. I plan on looking at it after the rebase :)

Cool. I am rebasing it now, will push fresh commits in sometime

helfi92

Seems like there's a small regression. Added some comments. When you get a chance, could you perform a rebase, please. Sorry for not reviewing this on Friday, I had to leave early.

helfi92 · 2019-06-03T22:08:55Z

ui/src/components/HookForm/index.jsx

@@ -387,6 +391,8 @@ export default class HookForm extends Component {
      hook.hookId &&
      hook.metadata.name &&
      hook.metadata.owner &&
+      hook.bindings &&
+      hook.bindings.length > 0 &&


STR:

Add 1 binding only

Save the page

Refresh the page

Delete the binding

Try to save

Expected: The save button is clickable
Actual: The save button is not clickable.

@helfi92 Can you configure this client id
mozilla-auth0/oauth2|firefoxaccounts|967134a2ed244f61b75935fc10b9119c|arshadkazmi42@gmail.com/taskcluster-web-server

I have refreshed my token

helfi92 · 2019-06-03T22:13:31Z

ui/src/components/PulseBindings/index.jsx

+    marginTop: 80,
+  },
+  deleteIcon: {
+    marginRight: -15,


marginRight: -theme.spacing.double,

helfi92 · 2019-06-03T22:14:15Z

ui/src/components/PulseBindings/index.jsx

+    marginRight: -15,
+    [theme.breakpoints.down('sm')]: {
+      marginRight: -14,
+    },


I think we can remove L31-33.

arshadkazmi42 · 2019-06-04T15:56:03Z

@helfi92 I think I broke something in rebase, its showing up 323 files change

Is there some way to revert this? Or I need to open a new PR?
I did the following

1. git fetch upstream
2. git rebase upstream/master (did this on bindings-hook-view branch)

arshadkazmi42 · 2019-06-04T16:08:36Z

@helfi92 I think I broke something in rebase, its showing up 323 files change

Is there some way to revert this? Or I need to open a new PR?
I did the following
1. git fetch upstream
2. git rebase upstream/master (did this on bindings-hook-view branch)

I managed to roll back the rebase.

Do I have to do git merge master ? on my bindings-hook-view branch?

Worker manager updates

helfi92 · 2019-06-04T18:29:20Z

@helfi92 I think I broke something in rebase, its showing up 323 files change

Sometimes git reflog can help but not always. You can read on it in your free time :)

Do I have to do git merge master ? on my bindings-hook-view branch?

I don't think you need to git merge. git rebase should do it (i think). I usually just do

# rebase current branch on upstream master
git pull upstream master --rebase

Once the rebase is done, you can git push origin bindings-hook-view --force.

Add workerPool (née workerType) creation interface (non-working)

This env var gets overridden by `infrastructure/terraform/modules/deployment/deployment.yaml` anyway.

…-web-ui Don't specify an explicit PORT for tc-web-ui

…/taskcluster into bindings-hook-view

arshadkazmi42 · 2019-06-05T08:24:26Z

I have done this

# rebase current branch on upstream master
git pull upstream master --rebase

Once the rebase is done, you can git push origin bindings-hook-view --force.

helfi92 · 2019-06-05T15:38:43Z

@arshadkazmi42 I think a merge commit was done. I am seeing other commits on this branch. Any chance you could open a separate PR instead with the change, please?

arshadkazmi42 · 2019-06-06T10:16:35Z

@arshadkazmi42 I think a merge commit was done. I am seeing other commits on this branch. Any chance you could open a separate PR instead with the change, please?

Sure. will do.

Add Bindings view in Hooks create page

2936bca

arshadkazmi42 requested a review from a team as a code owner May 12, 2019 18:40

helfi92 reviewed May 13, 2019

View reviewed changes

Create PulseBinding Component and use in PulseMessages and HookForm

641d42b

helfi92 self-requested a review May 13, 2019 21:21

helfi92 suggested changes May 15, 2019

View reviewed changes

add static props in PulseBindings component

b8f864b

helfi92 suggested changes May 17, 2019

View reviewed changes

Add binding to return from getDefinition

f266cfc

Add bindings to payload

5b9d224

helfi92 reviewed May 23, 2019

View reviewed changes

helfi92 self-requested a review May 23, 2019 17:46

helfi92 reviewed May 23, 2019

View reviewed changes

Change routingKeyPattern to pattern for bindings in payload

f4984a0

helfi92 self-requested a review May 24, 2019 12:52

helfi92 reviewed May 24, 2019

View reviewed changes

Change pattern to routingKeyPattern

7776395

helfi92 self-requested a review May 24, 2019 17:51

helfi92 reviewed May 29, 2019

View reviewed changes

helfi92 suggested changes Jun 3, 2019

View reviewed changes

Css cleanup, delete all bindings and save button enable fix

3e35cea

arshadkazmi42 requested a review from a team as a code owner June 4, 2019 15:55

arshadkazmi42 force-pushed the bindings-hook-view branch from 357e0f5 to 3e35cea Compare June 4, 2019 16:07

owlishDeveloper and others added 2 commits June 4, 2019 09:18

Add minor changes

a8980c3

Worker manager updates (taskcluster#831)

645cac1

Worker manager updates

owlishDeveloper and others added 14 commits June 4, 2019 13:13

Merge pull request taskcluster#814 from owlishDeveloper/bug1542905-3

9aa285d

Add workerPool (née workerType) creation interface (non-working)

Don't specify an explicit PORT for tc-web-ui

d28d642

This env var gets overridden by `infrastructure/terraform/modules/deployment/deployment.yaml` anyway.

Merge pull request taskcluster#836 from djmitche/no-explicit-PORT-for…

949fbc5

…-web-ui Don't specify an explicit PORT for tc-web-ui

Make setTimeout work with large timeout values (taskcluster#838)

21fd0ef

Add Bindings view in Hooks create page

989c444

Create PulseBinding Component and use in PulseMessages and HookForm

3bbbb11

add static props in PulseBindings component

76a6f49

Add binding to return from getDefinition

019d25c

Add bindings to payload

023761b

Change routingKeyPattern to pattern for bindings in payload

a31db51

Change pattern to routingKeyPattern

61a4307

Change fetch query in webserver and change UI to render fetched values

16d71d2

Css cleanup, delete all bindings and save button enable fix

badc0a3

Merge branch 'bindings-hook-view' of https://github.com/arshadkazmi42…

bf4c6e0

…/taskcluster into bindings-hook-view

djmitche mentioned this pull request Jun 5, 2019

[UI] Support triggering tasks in response to pulse messages #221

Closed

arshadkazmi42 mentioned this pull request Jun 6, 2019

Add PulseBinding Component and integrate it in HookForm and PulseMess… #849

Merged

helfi92 removed the request for review from a team June 7, 2019 15:12

helfi92 closed this Jul 8, 2019

Add Bindings view in Hooks create page #730

Add Bindings view in Hooks create page #730

Conversation

arshadkazmi42 commented May 12, 2019

Fixes #610

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helfi92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helfi92 left a comment

Choose a reason for hiding this comment

arshadkazmi42 commented May 17, 2019

helfi92 commented May 21, 2019

arshadkazmi42 commented May 22, 2019

helfi92 commented May 22, 2019

Choose a reason for hiding this comment

arshadkazmi42 May 23, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arshadkazmi42 commented May 24, 2019

arshadkazmi42 commented May 24, 2019

arshadkazmi42 commented May 24, 2019

helfi92 commented May 24, 2019

arshadkazmi42 commented May 24, 2019

helfi92 commented May 24, 2019

arshadkazmi42 commented May 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helfi92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arshadkazmi42 commented Jun 4, 2019 • edited

arshadkazmi42 commented Jun 4, 2019 • edited

helfi92 commented Jun 4, 2019 • edited

arshadkazmi42 commented Jun 5, 2019

helfi92 commented Jun 5, 2019

arshadkazmi42 commented Jun 6, 2019

arshadkazmi42 May 23, 2019 •

edited

arshadkazmi42 commented Jun 4, 2019 •

edited

arshadkazmi42 commented Jun 4, 2019 •

edited

helfi92 commented Jun 4, 2019 •

edited