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

provide streamlit config file inside docker #63

Merged
merged 2 commits into from
Jun 3, 2024

Conversation

Arslan-Siraj
Copy link
Contributor

@Arslan-Siraj Arslan-Siraj commented Jun 3, 2024

User description

  • provide streamlit config file inside docker for streamlit configuration as fileupload size etc.

PR Type

enhancement, configuration changes


Description

  • Updated URLs in pages/4_📖_Windows_executable.py to fetch markdown content from the OpenMS repository.
  • Changed the server port configuration in .streamlit/config.toml to 8501.
  • Added the Streamlit configuration file to the Docker image in the Dockerfile.

Changes walkthrough 📝

Relevant files
Enhancement
4_📖_Windows_executable.py
Update markdown content URLs to new repository                     

pages/4_📖_Windows_executable.py

  • Updated URLs for fetching markdown content to point to the OpenMS
    repository.
  • +2/-2     
    Configuration changes
    config.toml
    Update server port configuration in Streamlit config         

    .streamlit/config.toml

    • Changed the server port configuration to 8501.
    +1/-1     
    Dockerfile
    Include Streamlit configuration file in Docker image         

    Dockerfile

  • Added a line to copy the Streamlit configuration file into the Docker
    image.
  • +2/-0     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @qodo-merge-pro qodo-merge-pro bot added enhancement New feature or request configuration changes labels Jun 3, 2024
    Copy link

    qodo-merge-pro bot commented Jun 3, 2024

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are straightforward and limited to configuration and URL updates. The PR modifies URLs in a Python script, changes a port configuration in a TOML file, and updates the Dockerfile to include a new configuration file. These changes are not complex and should be easy to review for correctness.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The hardcoded URLs in the Python script might not be the most maintainable approach. Consider using environment variables or a configuration file to manage URLs.

    🔒 Security concerns

    No

    Copy link

    qodo-merge-pro bot commented Jun 3, 2024

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Define a base URL constant to avoid redundancy and potential errors

    To avoid redundancy and potential errors, consider defining the base URL as a constant and
    appending the specific paths for each tab.

    pages/4_📖_Windows_executable.py [57]

    -markdown_url = "https://raw.githubusercontent.com/OpenMS/streamlit-template/main/win_exe_with_embed_py.md"
    +BASE_URL = "https://raw.githubusercontent.com/OpenMS/streamlit-template/main/"
    +markdown_url = BASE_URL + "win_exe_with_embed_py.md"
     
    Suggestion importance[1-10]: 8

    Why: Defining a base URL as a constant reduces redundancy and potential errors in URL management across different sections of the code.

    8
    Enhancement
    Make the port number configurable via environment variables for flexibility

    Ensure that the port number is configurable via environment variables to allow flexibility
    in different deployment environments.

    .streamlit/config.toml [5]

    -port = 8501 # should be same as configured in deployment repo
    +port = ${PORT:-8501} # should be same as configured in deployment repo
     
    Suggestion importance[1-10]: 8

    Why: Making the port number configurable via environment variables enhances flexibility and adaptability in different deployment environments.

    8
    Possible issue
    Add error handling for the content retrieval from the URL

    Consider adding a check to ensure that fetch_markdown_content successfully retrieves the
    content from the URL. This can help handle cases where the URL might be incorrect or the
    server is down.

    pages/4_📖_Windows_executable.py [59]

     markdown_content = fetch_markdown_content(markdown_url)
    +if not markdown_content:
    +    st.error("Failed to retrieve content. Please check the URL or try again later.")
     
    Suggestion importance[1-10]: 7

    Why: Adding error handling for content retrieval is a good practice to ensure robustness, especially when dealing with external resources.

    7
    Best practice
    Add a .dockerignore file to exclude unnecessary files from the Docker build context

    Consider adding a .dockerignore file to exclude unnecessary files and directories from the
    Docker build context, which can help reduce the image size and build time.

    Dockerfile [118]

    +# Add a .dockerignore file to exclude unnecessary files
     COPY .streamlit/config.toml /app/.streamlit/config.toml
     
    Suggestion importance[1-10]: 7

    Why: Adding a .dockerignore file is a best practice to reduce Docker image size and build time by excluding unnecessary files.

    7

    @Arslan-Siraj Arslan-Siraj merged commit 703f46c into OpenMS:main Jun 3, 2024
    6 checks passed
    JohannesvKL pushed a commit to JohannesvKL/PTMScanner that referenced this pull request Dec 19, 2024
    provide streamlit config file inside docker
    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.

    1 participant