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

Add decorateBaseSchema function qlkube #454

Merged
merged 1 commit into from
May 3, 2021

Conversation

ralls0
Copy link
Member

@ralls0 ralls0 commented May 1, 2021

Description

New function has been added. decorateBaseSchema function allow to change the dynamic schema create on the fly by qlkube. This new function uses five parameters:

  • @param {*} targetQuery : Query wrapped int the extended object Type
  • @param {*} extendedType : Object type that should be extended
  • @param {*} argsNeeded : Arguments extracted from the father and passed from the wrapper
  • @param {*} nameWrapper : Custom name for the wrapper
  • @param {*} baseSchema : GraphQL Schema where the type and query are written

then a new GraphQLSchema object is returned.

How Has This Been Tested?

Below a test query used to test the new feature:

query test {
  itPolitoCrownlabsV1alpha2Instance (
    name: "green-tea-6831",
    namespace: "tenant-john-doe"
  )
  {
    spec {
      running
      templateCrownlabsPolitoItTemplateRef {
        name
        namespace
        templateWrapper{
          itPolitoCrownlabsV1alpha2Template {
            kind
            spec {
              prettyName
            }
          }
        }
      }
    }
  }
}

@ralls0 ralls0 requested a review from a team as a code owner May 1, 2021 06:39
@kingmakerbot
Copy link
Collaborator

Hi @ralls0. Thanks for your PR.

I am @kingmakerbot.
You can interact with me issuing a slash command in the first line of a comment.
Currently, I understand the following commands:

  • /rebase: Rebase this PR onto the master branch
  • /merge: Merge this PR into the master branch
  • /hold: Adds hold label to prevent merging with /merge
  • /unhold: Removes the hold label to allow merging with /merge
  • /deploy-staging: Deploy a staging environment to test this PR
  • /undeploy-staging: Manually undeploy the staging environment

Make sure this PR appears in the CrownLabs changelog, adding one of the following labels:

  • kind/breaking: 💥 Breaking Change
  • kind/feature: 🚀 New Feature
  • kind/bug: 🐛 Bug Fix
  • kind/cleanup: 🧹 Code Refactoring
  • kind/docs: 📝 Documentation

@ralls0 ralls0 added the kind/feature New feature or request label May 1, 2021
@ralls0 ralls0 force-pushed the ral/decorate_schema branch from c1bcc85 to ef5710c Compare May 1, 2021 06:59
@ralls0 ralls0 force-pushed the ral/decorate_schema branch 2 times, most recently from 3d17b73 to d3235a1 Compare May 1, 2021 07:28
const targetType = baseSchema.getQueryType().getFields()[targetQuery];
if (!targetType) return baseSchema;

nameWrapper = nameWrapper ? nameWrapper : 'fieldWrapper';
Copy link
Contributor

Choose a reason for hiding this comment

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

I would provide an error in case the nameWrapper is not defined, as you did with targetQuery

for (e of argsNeeded) {
newParent[e] = parent[e];
}
return newParent !== {} ? newParent : parent; // gestire errori
Copy link
Contributor

Choose a reason for hiding this comment

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

The !== {} check will always result false, if you need this check you should use a loop with Obejct.keys. But I believe that a better approach would be to throw an error if the needed prop doesn't exist on the parent.

(also please remove the // gestire errori part)

Copy link
Member Author

Choose a reason for hiding this comment

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

Being that argsNeeded is optional also newParent could be empty, isn't so?
May be Object.keys(newParent).length !==0 ? newParent : parent a good alternative?
I could add also a default value for the optional agrs.

@GabriFila
Copy link
Contributor

@ralls0 thanks for the PR! 😍
I added few comments, also please edit the title of the PR and remove the testA,testB part from the PR description

@ralls0 ralls0 changed the title Ral/decorate schema Add decorateBaseSchema function qlkube May 1, 2021
@ralls0 ralls0 force-pushed the ral/decorate_schema branch 3 times, most recently from 20c8c77 to 23f58a6 Compare May 3, 2021 07:02
@GabriFila
Copy link
Contributor

/rebase

Copy link
Contributor

@GabriFila GabriFila left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@QcFe QcFe left a comment

Choose a reason for hiding this comment

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

/lgtm

@giorio94
Copy link
Member

giorio94 commented May 3, 2021

/rebase

@kingmakerbot kingmakerbot force-pushed the ral/decorate_schema branch from 23f58a6 to 5598390 Compare May 3, 2021 15:41
@GabriFila
Copy link
Contributor

/deploy-staging

@kingmakerbot
Copy link
Collaborator

Your staging environment has been correctly deployed/updated!

@GabriFila
Copy link
Contributor

/merge

@GabriFila
Copy link
Contributor

/merge

@kingmakerbot kingmakerbot merged commit d694b99 into netgroup-polito:master May 3, 2021
@kingmakerbot
Copy link
Collaborator

Your staging environment has been correctly teared-down!

@ralls0 ralls0 deleted the ral/decorate_schema branch June 12, 2021 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants