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: add callbackurl to advise endpoint #1759

Merged
merged 1 commit into from
Sep 13, 2022
Merged

feat: add callbackurl to advise endpoint #1759

merged 1 commit into from
Sep 13, 2022

Conversation

KPostOffice
Copy link
Member

@KPostOffice KPostOffice commented May 16, 2022 •
edited
Loading

Related Issues and Dependencies

closes: #1756

This introduces a breaking change

  • No

This should yield a new module release

  • Yes
  • No

This Pull Request implements

Creation of secrets based on document id as well as posting results to webhook if already available.

@sesheta sesheta added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 16, 2022
@sesheta sesheta requested review from fridex and harshad16 May 16, 2022 21:33
@sesheta
Copy link
Member

sesheta commented May 16, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@sesheta sesheta added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 16, 2022
@KPostOffice KPostOffice marked this pull request as ready for review June 6, 2022 22:01
@sesheta sesheta removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 6, 2022
harshad16
harshad16 reviewed Jun 17, 2022
View reviewed changes
Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

lgtm,
just few comments

thoth/user_api/api_v1.py Outdated
value = json.dumps({"callbackurl": callbackurl, "Authorization": auth_header, "client_data": client_data})
entry_name = random.choices(string.ascii_letters, k=16)
body = [{"op": "add", "path": f"/data/{entry_name}", "value": value}]
k8.client.CoreV1Api.patch_namespaced_secret(
Copy link
Member

Choose a reason for hiding this comment

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

should we try to use common repo , so that if this kind action is needed somewhere else, we can use it ?
we would have to create a patch_secret using oc
Example: https://github.com/thoth-station/common/blob/5645625b9b7411a78187cd22b5a0410913e2145d/thoth/common/openshift.py#L1114

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think adding it to common is a good idea. I think creating a function in common will not remove any of the complexity from the call and it will just add another layer of abstraction.

thoth/user_api/api_v1.py Outdated Show resolved Hide resolved
@goern
Copy link
Member

goern commented Jun 22, 2022

what is the status of this?

@harshad16
Copy link
Member

/deploy

@sesheta sesheta added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 8, 2022
@KPostOffice
Copy link
Member Author

This should be good to go now.

@KPostOffice
Copy link
Member Author

@harshad16 PTAL

harshad16
harshad16 approved these changes Sep 7, 2022
View reviewed changes
Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
thanks 💯

@sesheta sesheta added the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2022
@sesheta
Copy link
Member

sesheta commented Sep 7, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: harshad16
To complete the pull request process, please assign sesheta after the PR has been reviewed.
You can assign the PR to them by writing /assign @sesheta in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@KPostOffice
Copy link
Member Author

Hmm, not sure who has permissions to merge this

@harshad16 harshad16 merged commit 8425671 into thoth-station:master Sep 13, 2022
@harshad16 harshad16 mentioned this pull request Sep 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harshad16 harshad16 harshad16 approved these changes

@fridex fridex Awaiting requested review from fridex

Assignees

@harshad16 harshad16

Labels
lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[3pt] Add logic for creating/patching callback url secrets and sending results
4 participants
@KPostOffice @sesheta @goern @harshad16