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

[Chore] - Fixed Markdown Lint and Removed Duplicated Code #124

Closed
wants to merge 3 commits into from

Conversation

adithyaakrishna
Copy link
Contributor

Description:

  • Adds superlinter workflow
  • Removed duplicate run for build check
  • Updated Dependency Review workflow

Fixes: #117

Copy link
Collaborator

alabulei1 commented Jul 3, 2023

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Overall Summary:

In this PR, there are several potential issues and errors that need attention.

  1. Conflicts resolution: Although it is mentioned that conflicts have been resolved, there is no explicit mention of how they were resolved in the patch. It is important to verify that the conflicts have been resolved correctly.

  2. Workflow trigger update: The change in .github/workflows/build-test.yml to trigger the workflow on additional events like opened, synchronize, reopened, labeled, and ready_for_review is a good improvement. However, the removal of the restriction to trigger the workflow only on the main branch may cause it to trigger on all branches, which might not be the desired behavior. Consider if this change aligns with your workflow requirements.

  3. Unused environment variables: The new workflow added in .github/workflows/lint.yml has two unused environment variables, DEFAULT_BRANCH and VALIDATE_ALL_CODEBASE. Verify if these variables are needed or if they can be removed to keep the workflow clean and efficient.

  4. Regex pattern verification: Double-check the regex pattern .*docs/.*md$ in the FILTER_REGEX_INCLUDE of the new workflow. Ensure that it correctly includes the files that need to be validated and consider if any other files should be included or excluded from the validation.

  5. Secrets configuration: Confirm that the required secrets, GITHUB_TOKEN, are properly set and accessible in the workflow. Lack of proper configuration may cause issues in accessing the necessary resources.

Additionally, there are a few findings worth noting:

  • The on event trigger for the workflow has been updated from [pull_request] to pull_request. Ensure that this change aligns with the intended behavior of the workflow.

  • The commit message mentions an update to the "Dependency Review Workflow," but the actual changes in the patch only modify the on event trigger. It is recommended to accurately reflect the changes made in the commit message.

  • The action name in the lint.yml file has been updated from github/super-linter/slim@v5 to super-linter/super-linter/slim@v5. Verify that this change is intentional and does not break any existing functionality or dependencies.

Details

Commit fc9f1905aab3b65560f75e422e0364033b354a88

Key changes:

  • Resolved conflicts in the branch with the pull request.
  • Updated the workflow in .github/workflows/build-test.yml to trigger on additional events: opened, synchronize, reopened, labeled, and ready_for_review.
  • Added a new workflow in .github/workflows/lint.yml to lint markdown format with the help of the super-linter action.

Potential problems:

  • It seems that the resolved conflicts are not mentioned explicitly in the patch. Make sure to verify that the conflicts have been resolved correctly.
  • The change in .github/workflows/build-test.yml removes the restriction to trigger the workflow only on the main branch. This may cause the workflow to trigger on all branches, which might not be the desired behavior.
  • In the new workflow added in .github/workflows/lint.yml, the environment variables DEFAULT_BRANCH and VALIDATE_ALL_CODEBASE are not used. Verify if they are needed or if they can be removed.
  • Check if the regex pattern .*docs/.*md$ in the FILTER_REGEX_INCLUDE is correct for the files that need to be validated. Double-check if any other files should be included or excluded from the validation.
  • Confirm that the required secrets, GITHUB_TOKEN, are properly set and accessible in the workflow.

Commit d565e2c4873351bb8efd654a5935e17a1b3c1d8f

Key Changes:

  • Updated the "on" event trigger for the workflow from [pull_request] to pull_request.
  • Added indentation for the pull_request event.

Potential Problems:

  • The on event trigger for the workflow has been modified. It is important to ensure that this change aligns with the intended behavior of the workflow.
  • The commit message mentions "Updated Dependency Review Workflow," but the actual changes in the patch only modify the on event trigger. It is unclear if there are any additional changes made to the workflow. The commit message should accurately reflect the changes made.

Commit 7e79ef554f357be81dc6635257ade3019652da29

Summary of Key Changes:

  • Updated the action name in the lint.yml file.

Potential Problems:

  • The action name has been updated from github/super-linter/slim@v5 to super-linter/super-linter/slim@v5. Make sure that this change is intentional and does not break any existing functionality or dependencies.

@alabulei1
Copy link
Collaborator

alabulei1 commented Jul 3, 2023

Much appreciated. But this is not an urgent issue. Can you please check this out? #93 (comment) Thanks.

Also, could you please update your work progress? We're waiting for your work for a long time.

@alabulei1
Copy link
Collaborator

Hi @adithyaakrishna

Please fix the conflicts.

@adithyaakrishna
Copy link
Contributor Author

@alabulei1 Done

@adithyaakrishna
Copy link
Contributor Author

The linting seems to fail, will fix it asap

Adithya Krishna added 3 commits July 25, 2023 21:10
Signed-off-by: Adithya Krishna <adikrish@redhat.com>
Signed-off-by: Adithya Krishna <adikrish@redhat.com>
Signed-off-by: Adithya Krishna <adikrish@redhat.com>
@adithyaakrishna
Copy link
Contributor Author

Will open a new PR, closing this for now

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.

Bug: The linter is not the same as the original repo
2 participants