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

Adding a pre-commit hook in Beckn core specification repository #401

Open
wants to merge 5 commits into
base: draft
Choose a base branch
from

Conversation

rajaneeshk90
Copy link
Collaborator

@rajaneeshk90 rajaneeshk90 commented Jan 22, 2024

Description:

Dear Team,

This pull request introduces a crucial enhancement to our repository by implementing a pre-commit hook. This pre-commit hook is designed to ensure the prevention of OpenAPI syntactical errors in specification before changes are committed.

Issue addressed by this PR:

#403
Summary of the Issue:
Need a pre-commit hook for Open API syntax validation for Beckn core specification. The implementation of this script should prevent the introduction of any syntactical errors into the core specification with each new commit.

Changes Made:

A new directory named pre-commit-scripts is added in the root of the Beckn core protocol specification. A shell script is added in the pre-commit-scripts directory which will be called when a user tries to create a commit. The added shell script checks if the relevant files(listed below) are valid OpenAPI documents. The addition of pre-commit script will significantly reduce the likelihood of syntactical errors in the specified files.

The files being validated are:

  1. api/meta/build/meta.yaml
  2. api/transaction/build/transaction.yaml
  3. api/registry/build/registry.yaml

How to configure the pre-commit scripts on your local machine:

Please follow the README.md file inside the pre-commit-scripts directory at the root of the protocol specification repository for setting up and configuring the hook locally.

Reviewer's guideline

[Note: Sample commands are provides in some of the below steps, these sample commands are tested on a unix machine. The users can choose to use the provided sample commands, but they are not restricted to it. They are free to use other commands to perform the below steps.]

  • Clone the protocol specification GitHub repository locally. Link to the repository is: https://github.com/beckn/protocol-specifications

  • Pull this PR on your local machine. Command: 'git fetch origin pull/401/head:test-pre-commit'. This command will create new branch named 'test-pre-commit' on your local machine.

  • Checkout on the 'test-pre-commit' branch of the cloned protocol specification repository. use the command 'git checkout test-pre-commit'. After successfully executing the checkout command, a new directory named 'pre-commit-scripts' gets created in the root of the repository.

  • Follow the steps mentioned in pre-commit-scripts/README.md to configure the pre-commit on your local machine.

  • Introducing a syntax error in the specification and committing the change:

    • Inside the api/transaction/build/transaction.yaml change the components.schemas.XInput.properties.Form to #/components/schemas/TestForm
    • Changing to TestForm would introduce an error because TestForm is not defined in the api/transaction/build/transaction.yaml file
    • Save the api/transaction/build/transaction.yaml file, try to commit this change, the commit command should fail.
  • Making a change in the specification (api/transaction/build/transaction.yaml) file which keeps the specification file a valid openAPI document.

    • Add a new component User in the api/transaction/build/transaction.yaml at components.schemas, code is provided below
              User:
                description: 'This is a test chnage'
                type: object
                properties:
                  name:
                    type: string
    • save the api/transaction/build/transaction.yaml file
    • Try to commit the change done in the api/transaction/build/transaction.yaml file, the commit command should pass
  • [Note: It is not recommended to directly change the api/transaction/build/transaction.yaml file if we are adding/modifying the Beckn core protocol specification. To know how to contribute to the Beckn core protocol specification, please follow the instructions on the Beckn core protocol specification page. We are making direct changes in api/transaction/build/transaction.yaml here only to test this PR]

Benefits of the pre-commit script

  1. Bug Prevention: Identifies potential syntactical errors in any changes made to the Beckn core specification, and does not allow the users to create commits if the change has syntactical errors. It prevents any invalid openAPI changes made in Beckn core protocol specification before they are committed reducing the likelihood of bugs making their way into the Beckn core protocol specification.
  2. Time Savings: Saves time and effort by catching issues early, minimising the need for time-consuming debugging later.

Limitations of the pre-commit script:

if the users use '--no-verify' with the git commit command, they can skip the openAPI validation checks performed by the pre-commit-script.

How to tackle the limitations:

We can add a similar check on GitHub protocol specification repository in the form of a GitHub action. The GitHub action would run after a commit is pushed to the GitHub Beckn protocol specification repository.

I welcome any feedback, suggestions, or improvements you may have regarding this pull request.

Thank you for your time and consideration.
Rajaneesh

@rajaneeshk90
Copy link
Collaborator Author

rajaneeshk90 commented Jan 24, 2024

solves issue #403

Summary of the Issue:
Need a pre-commit hook for Open API syntax validation for Beckn core specification. The implementation of this script should prevent the introduction of any syntactical errors into the core specification with each new commit.

@rajaneeshk90
Copy link
Collaborator Author

@vbabuEM Please review this PR.

Copy link

@vbabuEM vbabuEM left a comment

Choose a reason for hiding this comment

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

Rajaneesh, should we also make the pre-commit file itself as executable? If so please add that line to the instruction somewhere. Thanks

@rajaneeshk90
Copy link
Collaborator Author

Thank you, @vbabuEM, for your review of the PR.

I appreciate your insights, and I've taken your feedback into consideration. To address the point you raised, I've added an extra line to the README.md instructions.

Thanks again for your time and input.

Best regards,
Rajaneesh

@ravi-prakash-v
Copy link
Collaborator

Hi @rajaneeshk90 please provide Testing steps in your PR.

@rajaneeshk90 rajaneeshk90 changed the title Adding pre-commit hook Adding a pre-commit hook in Beckn core specification repository Feb 14, 2024
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.

3 participants