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

fixes graphql-introspection #1433

Merged
merged 2 commits into from
Jan 15, 2024

Conversation

aialok
Copy link
Contributor

@aialok aialok commented Jan 15, 2024

What kind of change does this PR introduce?

  • It will fix the graph introspection that is currently failing.

Issue Number:

Fixes #1429

Did you add tests for your changes?

  • No

Snapshots/Videos:

If relevant, did you update the documentation?

Summary

  • It will fix the graphql-introspection that is currently failing. However, we need to be alert about the fact that whenever we merge the develop branch of talawa-admin into the main branch, we must first merge the develop branch of talawa-api into main.

Does this PR introduce a breaking change?

Other information

Have you read the contributing guide?

@aialok aialok requested a review from palisadoes as a code owner January 15, 2024 14:38
Copy link

Our Pull Request Approval Process

We have these basic policies to make the approval process smoother for our volunteer team.

Testing Your Code

Please make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:

  1. The overall code coverage drops below the target threshold of the repository
  2. Any file in the pull request has code coverage levels below the repository threshold
  3. Merge conflicts

The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing.

Reviewers

When your PR has been assigned reviewers contact them to get your code reviewed and approved via:

  1. comments in this PR or
  2. our slack channel

Reviewing Your Code

Your reviewer(s) will have the following roles:

  1. arbitrators of future discussions with other contributors about the validity of your changes
  2. point of contact for evaluating the validity of your work
  3. person who verifies matching issues by others that should be closed.
  4. person who gives general guidance in fixing your tests

CONTRIBUTING.md

Read our CONTRIBUTING.md file. Most importantly:

  1. PRs with issues not assigned to you will be closed by the reviewer
  2. Fix the first comment in the PR so that each issue listed automatically closes

Other

  1. 🎯 Please be considerate of our volunteers' time. Contacting the person who assigned the reviewers is not advised unless they ask for your input. Do not @ the person who did the assignment otherwise.
  2. Read the CONTRIBUTING.md file make

@aialok
Copy link
Contributor Author

aialok commented Jan 15, 2024

Hey @palisadoes,
The branch issue has been resolved, and we are now comparing the develop branches of both repositories. However, a new bug has emerged. The GraphQL introspection is still failing because many mutations and queries in talawa-admin are not present in the develop branch of talawa-api. But these mutations and queries are available in the main branch. This mismatch is causing the GraphQL inspector to fail.

Here are one of the of the example:

  1. See the first error Cannot query fieldcreateEventProject on type mutation .

inspector

  1. See createEventProject exist in mutation talawa-admin .

talawa-admin

  1. But there is no such mutation or text in the talawa-api develop branch

talawa-api

  1. Look we have createEventProject mutations exist in the talawa-api main branch.

talawa-api-create-event-main

Screenshot from 2024-01-15 21-14-40

  1. But there is no such file exist in the talawa-api develop branch.

Screenshot from 2024-01-15 21-13-58

  • These are the reason why our graphql-inspector is getting failed. I tried checking out the commit history but could not found anything related to the removal of these file or why these files are not there.
  • The test failed because of isPublic changed to userRegistrationRequired is now resolved.

Thank You : )

@palisadoes
Copy link
Contributor

It's failing. You may need to be using $FULL_BRANCH_NAME instead. The echo of $BRANCH_NAME isn't working

You'll need to do further commits. If it's working the introspection test must pass as part of this PR

@aialok
Copy link
Contributor Author

aialok commented Jan 15, 2024

It's failing. You may need to be using $FULL_BRANCH_NAME instead. The echo of $BRANCH_NAME isn't working

You'll need to do further commits. If it's working the introspection test must pass as part of this PR

  • The problem is not with FULL_BRANCH_NAME but in the talawa-api develop branch. See above comment I have described the bug.
  • For fixing the graphql-introspection we need to change in the talawa-api develop branch.

Copy link

codecov bot commented Jan 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (16de771) 97.34% compared to head (216198e) 97.34%.
Report is 1 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1433   +/-   ##
========================================
  Coverage    97.34%   97.34%           
========================================
  Files          136      136           
  Lines         3572     3572           
  Branches      1039     1039           
========================================
  Hits          3477     3477           
  Misses          90       90           
  Partials         5        5           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@palisadoes
Copy link
Contributor

  1. That's not entirely true. There was an error with FULL_BRANCH_NAME which you seem to have solved. I'll merge this.
  2. Please open an equivalent issue in the API repo to complete this task.

@palisadoes
Copy link
Contributor

Also, merge this file into push.yml in the API repo. We have been trying to make it easier to troubleshoot events, and this will help.

I see the hard coded develop in the file. Is that the source of the issue? I can't see why it is now as both repos default to develop, but I can see that being an issue in future.

I'll need an answer before I merge this

Are you sure this is the source of the current problem?

@aialok
Copy link
Contributor Author

aialok commented Jan 15, 2024

Also, merge this file into push.yml in the API repo. We have been trying to make it easier to troubleshoot events, and this will help.

I see the hard coded develop in the file. Is that the source of the issue? I can't see why it is now as both repos default to develop, but I can see that being an issue in future.

I'll need an answer before I merge this

Are you sure this is the source of the current problem?

  • Can you please provide more details about the merge in push.yml? Do I need to merge the GraphQL Inspector?

  • Regarding the issue in the inspect.yml file, I don't believe our current problem is related to inspect.yml file because in talawa-api we are missing some of the mutation and queries which is present in the main branch of talawa-api but no in the develop. However, after checking the workflow history, I found that it's failing with every push. we need to fix the inspect.yml file too and we also need to add dynamic branch so that in future when we doing push to main branch. it does not create any issue.

@palisadoes
Copy link
Contributor

  1. In the API repo, the commands in the inspect.yml file are invoked with every push.
  2. We have a separate push.yml file that is also invoked with every push.
  3. We need to move the YAML code in the inspect.yml to the push.yml file and delete the inspect.yml file
  4. When this is done, the name of the job needs to be changed from test to something indicative of doing introspection testing.
  5. It the migrated YAML code needs to:
    1. be dependent on the Push-Workflow job in the push.yml file
    2. ensure that the inspect.yml file's code automatically adjusts to the branch
  6. This will mean that developers will only need to edit one file for all push related changes to our GitHub actions

Does this make sense to you for the long term health of the project? We don't want a solution that we'll need to fix repeatedly. Are these suggestions clear?

@aialok
Copy link
Contributor Author

aialok commented Jan 15, 2024

  1. In the API repo, the commands in the inspect.yml file are invoked with every push.
  2. We have a separate push.yml file that is also invoked with every push.
  3. We need to move the YAML code in the inspect.yml to the push.yml file and delete the inspect.yml file
  4. When this is done, the name of the job needs to be changed from test to something indicative of doing introspection testing.
  5. It the migrated YAML code needs to:
    1. be dependent on the Push-Workflow job in the push.yml file
    2. ensure that the inspect.yml file's code automatically adjusts to the branch
  6. This will mean that developers will only need to edit one file for all push related changes to our GitHub actions

Does this make sense to you for the long term health of the project? We don't want a solution that we'll need to fix repeatedly. Are these suggestions clear?

Yeah, got it. It helps us to manage our push workflow more easily. 🚀

@aialok
Copy link
Contributor Author

aialok commented Jan 15, 2024

Should I create a new issue for the inspect.yml file merging?
If this issue get resolved then merge it.
I will create new issue for the graph-introspection in talawa-api.

@palisadoes
Copy link
Contributor

Should I create a new issue for the inspect.yml file merging? If this issue get resolved then merge it. I will create new issue for the graph-introspection in talawa-api.

Yes, a single issue to integrate inspect.yml into push.yml and to fix the automatic branch detection that will be required

@palisadoes
Copy link
Contributor

Why is this the introspection test failing in this PR if it is supposed to be pulling from the develop schema file in the API, and the API is creating a schema file in develop with its workflows?

@aialok
Copy link
Contributor Author

aialok commented Jan 15, 2024

Why is this the introspection test failing in this PR if it is supposed to be pulling from the develop schema file in the API, and the API is creating a schema file in develop with its workflows?

As I mentioned earlier in the above comment, some queries and mutation files are missing in the API develop branch. When the workflow runs for auto-generation, it only updates or generates files that are present. However, those schema, mutations, and queries exist in talawa-admin. And because of this test are failing.

Although the test failure related to isPublic being changed to userRegistrationRequired is resolved, other queries and mutations are causing issues due to their absence in the API."

#1433 (comment)

@palisadoes
Copy link
Contributor

OK, thanks for reiterating the rationale. Please open the other issue and ask to be assigned to it

@palisadoes palisadoes merged commit 5bae8f5 into PalisadoesFoundation:develop Jan 15, 2024
8 of 10 checks passed
@aialok
Copy link
Contributor Author

aialok commented Jan 15, 2024

@palisadoes, I've reviewed the commit history and discovered that there's no need to create a new issue for this. The mutations and queries were deleted as part of deprecating the task and project features. In talawa-api the issue is resolved already.

For Talawa-Admin, the problem still persists (#1362). Once this issue is resolved, our graphql-introspection tests will pass successfully 🎉

Talawa-API

Talawa-Admin

@palisadoes
Copy link
Contributor

It's not just the mutations. We still need to make sure the GitHub action in the API automatically adjusts for the branch and consolidate the YAML into a single file. Please open an issue to do this.

@aialok
Copy link
Contributor Author

aialok commented Jan 15, 2024

It's not just the mutations. We still need to make sure the GitHub action in the API automatically adjusts for the branch and consolidate the YAML into a single file. Please open an issue to do this.

  • Sure, I will be creating the issue.

  • Regarding the graphql-introspection failed, it is only cause by the deprecating the project/task from talawa-api. If in talawa-admin depracating feature project/task got resolved then our graphql-introspection will pass.

@palisadoes
Copy link
Contributor

Understood. Please proceed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants