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.

Sign up for GitHub

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

Jump to bottom

feat: unify pipeline trigger chart endpoint with credit chart #228

Merged
merged 4 commits into from
Jul 26, 2024
Merged

feat: unify pipeline trigger chart endpoint with credit chart #228

merged 4 commits into from
Jul 26, 2024

Conversation

jvallesm
Copy link
Collaborator

@jvallesm jvallesm commented Jul 25, 2024 •
edited
Loading

Because

  • Pipeline and credit endpoints have different params and schemas.
  • Some of the information served by the existing endpoints will be handled by the logging feature.

This commit

@jvallesm jvallesm self-assigned this Jul 25, 2024
@linear Linear
Copy link

linear bot commented Jul 25, 2024

INS-4936 Unify pipeline and credit chart endpoint contracts

@jvallesm jvallesm force-pushed the jvalles/ins-4936-unify-pipeline-and-credit-chart-endpoint-contracts branch 2 times, most recently from ed308ce to 5751b66 Compare July 25, 2024 10:22
@jvallesm jvallesm force-pushed the jvalles/ins-4936-unify-pipeline-and-credit-chart-endpoint-contracts branch from 5751b66 to c58f6f2 Compare July 25, 2024 10:24
@jvallesm
Copy link
Collaborator Author

jvallesm commented Jul 25, 2024 •
edited
Loading

QA

$ curl 'localhost:8080/v1beta/metrics/vdp/pipeline/charts?namespaceId=jvallesm&start=2024-07-22T22:00:00.000Z&stop=2024-07-23T22:00:00.000Z&aggregationWindow=1h' \
  -H 'Authorization: Bearer instill_sk_1Myb31r6YsjVt8In2zf8vRvq6wUpfvRq' | jq


  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   742  100   742    0     0  38601      0 --:--:-- --:--:-- --:--:-- 39052
{
  "pipelineTriggerChartRecords": [
    {
      "timeBuckets": [
        "2024-07-22T23:00:00Z",
        "2024-07-23T00:00:00Z",
        "2024-07-23T01:00:00Z",
        "2024-07-23T02:00:00Z",
        "2024-07-23T03:00:00Z",
        "2024-07-23T04:00:00Z",
        "2024-07-23T05:00:00Z",
        "2024-07-23T06:00:00Z",
        "2024-07-23T07:00:00Z",
        "2024-07-23T08:00:00Z",
        "2024-07-23T09:00:00Z",
        "2024-07-23T10:00:00Z",
        "2024-07-23T11:00:00Z",
        "2024-07-23T12:00:00Z",
        "2024-07-23T13:00:00Z",
        "2024-07-23T14:00:00Z",
        "2024-07-23T15:00:00Z",
        "2024-07-23T16:00:00Z",
        "2024-07-23T17:00:00Z",
        "2024-07-23T18:00:00Z",
        "2024-07-23T19:00:00Z",
        "2024-07-23T20:00:00Z",
        "2024-07-23T21:00:00Z",
        "2024-07-23T22:00:00Z"
      ],
      "triggerCounts": [
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        8,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0,
        0
      ],
      "namespaceId": "jvallesm"
    }
  ]
}

@jvallesm
Copy link
Collaborator Author

QA

$ curl 'localhost:8080/v1beta/metrics/vdp/pipeline/trigger-count?namespaceId=jvallesm&start=2024-07-22T22:00:00.000Z&stop=2024-07-25T22:00:00.000Z' \
  -H 'Authorization: Bearer instill_sk_1Myb31r6YsjVt8In2zf8vRvq6wUpfvRq' | jq

  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   122  100   122    0     0   7058      0 --:--:-- --:--:-- --:--:--  7176
{
  "pipelineTriggerCounts": [
    {
      "triggerCount": 8,
      "status": "STATUS_COMPLETED"
    },
    {
      "triggerCount": 2,
      "status": "STATUS_ERRORED"
    }
  ]
}

@jvallesm jvallesm marked this pull request as ready for review July 25, 2024 12:10
jvallesm
jvallesm commented Jul 25, 2024
View reviewed changes
pkg/repository/repository.go
Comment on lines 67 to 70
listOwners(ctx context.Context, ownerType string, pageSize int, pageToken string, filter filtering.Filter) ([]*datamodel.Owner, int64, string, error)
createOwner(ctx context.Context, ownerType string, user *datamodel.Owner) error
getOwner(ctx context.Context, ownerType string, id string, includeAvatar bool) (*datamodel.Owner, error)
getOwnerByUID(ctx context.Context, ownerType string, uid uuid.UUID) (*datamodel.Owner, error)
updateOwner(ctx context.Context, ownerType string, id string, user *datamodel.Owner) error
deleteOwner(ctx context.Context, ownerType string, id string) error
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

💭 @donch1989 if owner ID is unique perhaps we don't need the type checks in these methods and we can delegate on the service layer to check the authenticated user permissions to perform these actions.

jvallesm
jvallesm commented Jul 25, 2024
View reviewed changes
pkg/repository/repository.go
@@ -250,12 +285,11 @@ func (r *repository) createOwner(ctx context.Context, ownerType string, owner *d
return nil
}

func (r *repository) getOwner(ctx context.Context, ownerType string, id string, includeAvatar bool) (*datamodel.Owner, error) {

func (r *repository) GetOwner(ctx context.Context, id string, includeAvatar bool) (*datamodel.Owner, error) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@donch1989 I mentioned this change in your PR yesterday, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we can expose it.

donch1989
donch1989 approved these changes Jul 26, 2024
View reviewed changes
Copy link
Member

@donch1989 donch1989 left a comment

Choose a reason for hiding this comment

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

LGTM

@donch1989 donch1989 merged commit c2ccc17 into main Jul 26, 2024
12 checks passed
@donch1989 donch1989 deleted the jvalles/ins-4936-unify-pipeline-and-credit-chart-endpoint-contracts branch July 26, 2024 04:31
@droplet-bot droplet-bot mentioned this pull request Jul 26, 2024
Merged
donch1989 pushed a commit that referenced this pull request Jul 31, 2024
@droplet-bot
chore(main): release 0.19.0-beta (#225)
0a6fcee 
🤖 I have created a release *beep* *boop*
---


##
[0.19.0-beta](v0.18.1-beta...v0.19.0-beta)
(2024-07-29)


### Features

* add private endpoint `CheckNamespaceByUIDAdmin`
([#227](#227))
([274d365](274d365))
* **mgmt:** add knowledge base acl model
([#226](#226))
([b4b813e](b4b813e))
* unify pipeline trigger chart endpoint with credit chart
([#228](#228))
([c2ccc17](c2ccc17))
* use explicit user_id and organization_id in request params
([#224](#224))
([3388190](3388190))


### Bug Fixes

* restore pipeline dashboard enpoints
([#230](#230))
([02e32df](02e32df))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@donch1989 donch1989 donch1989 approved these changes

@xiaofei-du xiaofei-du Awaiting requested review from xiaofei-du xiaofei-du is a code owner

@pinglin pinglin Awaiting requested review from pinglin pinglin is a code owner

Assignees

@jvallesm jvallesm

Labels
instill core
Projects
No open projects
🔮 Instill Core
Status: 👋 Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jvallesm @donch1989 @droplet-bot