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

fix(admin): make config changes to pass through gog middlewares (#8014) #8442

Merged
merged 2 commits into from
Nov 23, 2022

Conversation

joshua-goldstein
Copy link
Contributor

@joshua-goldstein joshua-goldstein commented Nov 19, 2022

Cherry pick from #8014

In #7955 the team made a change to the way lambda scripts were loaded into Dgraph. This PR had a small conflict with that PR. Any change related to #7955 should be made in another PR.

Currently, guardians of any namespace can enable/disable query logging, update the cache parameters, etc.
This operation should only be allowed to the guardian of the galaxy.

(cherry picked from commit 6737dfd)
@CLAassistant
Copy link

CLAassistant commented Nov 19, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ joshua-goldstein
❌ NamanJain8
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Nov 19, 2022
@coveralls
Copy link

coveralls commented Nov 19, 2022

Coverage Status

Coverage remained the same at 37.182% when pulling 7f18f85 on cherry-pick-8014 into b975b98 on main.

@joshua-goldstein joshua-goldstein added the cherry-pick-22.0.2 Label for all cherry-picks for 22.0.2 Release label Nov 19, 2022
@MichelDiz
Copy link
Contributor

@joshua-goldstein Please, sign the CLA.

FYI. Yes, we need getLambdaScript 100%. It is used in the cloud. If you remove it it will break the entire cloud.
What happened to make you wonder? Is it breaking some test? or CI?

@joshua-goldstein
Copy link
Contributor Author

joshua-goldstein commented Nov 21, 2022

@joshua-goldstein Please, sign the CLA.

FYI. Yes, we need getLambdaScript 100%. It is used in the cloud. If you remove it it will break the entire cloud. What happened to make you wonder? Is it breaking some test? or CI?

@MichelDiz I have already signed the CLA, it is giving this message because this is a cherry pick from other commits that have other contributors.

To your main point: this is currently not in the main branch. See here. Presumably if we want to base the cloud on v22.0.1 we will have to bring in other changes not related to this PR.

@MichelDiz
Copy link
Contributor

I see, yep they have changed a lot of places #7955
Also need to continue the change in the lambda repo.

In 21.03 (and previous versions) we stored the lambda scripts directly via flag in the binary. This change to GraphQL makes easy to deal with the JSscript(it is stored in bas64). This seemed obvious to me. It's a change from 16 months ago. I assumed it was already in effect.

As you can see, it is present in v21.09-slash(a cloud version) https://github.com/dgraph-io/dgraph/blob/release/v21.09-slash/graphql/admin/admin.go

@joshua-goldstein joshua-goldstein marked this pull request as ready for review November 21, 2022 19:44
@joshua-goldstein joshua-goldstein self-assigned this Nov 21, 2022
@meghalims meghalims self-requested a review November 23, 2022 23:29
Copy link
Contributor

@meghalims meghalims left a comment

Choose a reason for hiding this comment

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

LGTM. Please merge this Josh

@joshua-goldstein joshua-goldstein merged commit 4f3420d into main Nov 23, 2022
@joshua-goldstein joshua-goldstein deleted the cherry-pick-8014 branch November 23, 2022 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph. cherry-pick-22.0.2 Label for all cherry-picks for 22.0.2 Release
Development

Successfully merging this pull request may close these issues.

7 participants