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

feat: Update requirements and add GitHub client for organization and … #5

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

venkatamutyala
Copy link
Contributor

@venkatamutyala venkatamutyala commented Nov 21, 2024

User description

…repository management

feat: Update child pages for each github org to include details about each org for searchability


PR Type

enhancement, documentation


Description

  • Added a new GitHubClient class to manage GitHub organizations and repositories, including fetching data and generating markdown documentation.
  • Enhanced the main script to integrate with GitHubClient and GetOutlineClient, adding retry mechanisms for document operations.
  • Updated the README to specify required permissions for the GitHub token.
  • Updated dependencies in requirements.txt, including the addition of the tenacity library for retry functionality.

Changes walkthrough 📝

Relevant files
Enhancement
github.py
Add GitHub client for organization and repository management

app/github.py

  • Introduced GitHubClient class for managing GitHub organizations and
    repositories.
  • Implemented methods to fetch organizations, repositories, and topics.
  • Added markdown generation for organizations and repositories.
  • Integrated error handling and logging.
  • +131/-0 
    main.py
    Enhance main script with GitHub and Outline integration   

    app/main.py

  • Integrated GitHubClient and GetOutlineClient for document updates.
  • Added retry mechanisms for document creation and updates.
  • Enhanced logging and error handling.
  • Refactored main function to update GitHub documentation.
  • +67/-56 
    Documentation
    README.md
    Update README with GitHub token permissions                           

    README.md

  • Updated environment variable instructions for GITHUB_TOKEN.
  • Specified required permissions for GitHub token.
  • +1/-1     
    Dependencies
    requirements.txt
    Update dependencies for enhanced functionality                     

    requirements.txt

  • Updated glueops-helpers to version 0.6.0.
  • Added tenacity library for retry functionality.
  • +2/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    …repository management
    
    feat: Update child pages for each github org to include details about each org for searchability
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 Security concerns

    Token Exposure:
    The GitHub token and GetOutline API token are being passed directly in request headers. While using HTTPS, consider implementing additional security measures like token rotation or using environment-specific tokens. Also, the error logging could potentially expose sensitive information in the error messages.

    ⚡ Recommended focus areas for review

    Code Smell
    The generate_markdown method is defined as a static method but uses the logger instance variable. This could lead to undefined behavior.

    Duplicate Code
    The pagination handling logic is duplicated in get_organizations and get_repositories methods. Consider extracting to a common helper method.

    Error Handling
    The retry decorators use fixed values for attempts and wait times. Consider making these configurable through environment variables.

    Performance Issue
    The get_repository_topics method makes individual API calls for each repository, which could lead to rate limiting with many repositories.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix incorrect method definition in class by adding missing self parameter

    The generate_markdown method should be an instance method of the GitHubClient class,
    not a standalone function. Add self as the first parameter.

    app/github.py [48-49]

    -def generate_markdown(github_orgs):
    +def generate_markdown(self, github_orgs):
         logger.debug("Generating markdown for organizations.")
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This is a critical bug fix. The method is defined within a class but lacks the 'self' parameter, which would cause runtime errors when called as an instance method.

    9
    Fix content overwrite by using concatenation instead of reassignment

    The markdown content is being incorrectly overwritten. Remove the second assignment
    to markdown_content to preserve the auto-generation notice.

    app/github.py [50-51]

     markdown_content = "> This page is automatically generated. Any manual changes will be lost. See: https://github.com/GlueOps/getoutline-docs-update-github \n\n"
    -markdown_content = "# Full list of GitHub Organizations\n\n"
    +markdown_content += "# Full list of GitHub Organizations\n\n"
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This fixes a significant bug where the auto-generation notice is being overwritten, causing important information to be lost in the generated documentation.

    8
    General
    Fix variable naming to follow Python naming conventions

    The GetOutlineClient class name is incorrectly used as a variable. Use lowercase for
    the variable name following Python naming conventions.

    app/main.py [100]

    -GetOutlineClient = glueops.getoutline.GetOutlineClient(GETOUTLINE_API_URL, GETOUTLINE_DOCUMENT_ID, GETOUTLINE_API_TOKEN)
    +getoutline_client = glueops.getoutline.GetOutlineClient(GETOUTLINE_API_URL, GETOUTLINE_DOCUMENT_ID, GETOUTLINE_API_TOKEN)
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: This improves code readability and maintainability by following Python naming conventions, though it doesn't affect functionality.

    5

    💡 Need additional feedback ? start a PR chat

    Copy link

    @fernandoataoldotcom fernandoataoldotcom left a comment

    Choose a reason for hiding this comment

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

    lgtm

    @venkatamutyala venkatamutyala merged commit a19230a into main Nov 21, 2024
    3 checks passed
    @venkatamutyala venkatamutyala deleted the feat/doc-improvements branch November 21, 2024 20:41
    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.

    2 participants