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

Specify Cache Directory in Azure DevOps in Lambda Function Setup #450

Merged

Conversation

koid
Copy link
Contributor

@koid koid commented Nov 15, 2023

PR Type:

Documentation


PR Description:

This PR updates the documentation to include an additional step in the Lambda function setup process. The new step instructs users to specify the AZURE_DEVOPS_CACHE_DIR environment variable in the Lambda function to a writable location such as /tmp. This change is in response to issue #443.


PR Main Files Walkthrough:

files:
  • INSTALL.md: Added a new step in the Lambda function setup process instructing users to specify the AZURE_DEVOPS_CACHE_DIR environment variable to a writable location. The step numbering was also updated accordingly.

@koid
Copy link
Contributor Author

koid commented Nov 15, 2023

/describe

@github-actions github-actions bot changed the title specify the cache directory in Azure DevOps Specify Cache Directory in Azure DevOps Nov 15, 2023
Copy link
Contributor

@barnett-yuxiang barnett-yuxiang left a comment

Choose a reason for hiding this comment

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

May I ask, in general programming habits, the constant string is written in index or defined externally, how to make decisions?

@koid
Copy link
Contributor Author

koid commented Nov 15, 2023

Thank you for the question.

I also believe that it should be defined externally.
On the other hand, I am uncertain whether it is appropriate to define this issue in configuration.toml as a global application setting, considering that it is a serverless-specific problem.

I think there are other ways as well:

  • Add instructions to the Installation Guide to set AWS Lambda environment variables (without modifying the code.)
  • Define it in Dockerfile.lambda as an ARG with default values (allowing override with build args).

Which method would be appropriate?

@barnett-yuxiang
Copy link
Contributor

Thank you for the question.

I also believe that it should be defined externally. On the other hand, I am uncertain whether it is appropriate to define this issue in configuration.toml as a global application setting, considering that it is a serverless-specific problem.

I think there are other ways as well:

  • Add instructions to the Installation Guide to set AWS Lambda environment variables (without modifying the code.)
  • Define it in Dockerfile.lambda as an ARG with default values (allowing override with build args).

Which method would be appropriate?

Personally, I agree with the first one, but you can also listen to the owner. Does pr-agent have a division of owner CR?

@koid
Copy link
Contributor Author

koid commented Nov 15, 2023

Thank you, I'll limit the updates to the Installation Guide for now.

@koid koid force-pushed the fix/specify-cache-directory-in-azure-devops branch from b95e75f to 172b5f0 Compare November 15, 2023 04:22
@koid
Copy link
Contributor Author

koid commented Nov 15, 2023

/describe

@github-actions github-actions bot changed the title Specify Cache Directory in Azure DevOps Specify Cache Directory in Azure DevOps in Lambda Function Setup Nov 15, 2023
@github-actions github-actions bot added documentation Improvements or additions to documentation and removed Bug fix labels Nov 15, 2023
@mrT23
Copy link
Collaborator

mrT23 commented Nov 15, 2023

/review

Copy link
Contributor

PR Analysis

  • 🎯 Main theme: Update documentation to specify cache directory in Azure DevOps in Lambda function setup
  • 📝 PR summary: This PR updates the documentation to include an additional step in the Lambda function setup process. The new step instructs users to specify the AZURE_DEVOPS_CACHE_DIR environment variable in the Lambda function to a writable location such as /tmp.
  • 📌 Type of PR: Documentation
  • 🧪 Relevant tests added: No
  • ⏱️ Estimated effort to review [1-5]: 1, because the PR only includes a minor documentation change and does not involve any code modifications.
  • 🔒 Security concerns: No

PR Feedback

  • 💡 General suggestions: The PR is straightforward and clear. It provides a necessary update to the documentation, ensuring users specify the AZURE_DEVOPS_CACHE_DIR environment variable in the Lambda function setup. This will help users avoid potential issues in the future.

  • 🤖 Code feedback:

How to use

To invoke the PR-Agent, add a comment using one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest improvements to the code in the PR. Extended mode employs several calls, and provides a more thorough feedback.
/ask <QUESTION>: Pose a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.

To edit any configuration parameter from configuration.toml, add --config_path=new_value
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, use the /config command.

@barnett-yuxiang
Copy link
Contributor

@mrT23 Excuse me, may I ask how your Git Action above triggers PR Analysis work?

🌹

@mrT23
Copy link
Collaborator

mrT23 commented Nov 16, 2023

@mrT23
Copy link
Collaborator

mrT23 commented Nov 19, 2023

@koid
i dont understand this PR.
how 'AZURE_DEVOPS_CACHE_DIR` is related to lambda function deployed on AWS ?

@mrT23
Copy link
Collaborator

mrT23 commented Nov 28, 2023

@koid is this PR alive, or should I close it ?

i repeat my question - how 'AZURE_DEVOPS_CACHE_DIR` is related to lambda function deployed on AWS ?

@koid
Copy link
Contributor Author

koid commented Nov 29, 2023

I'm sorry, I noticed the mention late.

In azure-devops, if the cache directory is not explicitly specified, it will be created under the home directory of the executing user.

In the case of AWS Lambda, the executing user does not have write permissions for the home directory, leading to a failure in creating the cache directory.

To address this issue, instructions on specifying a path where the executing user has write permissions as the cache directory are added to the README.

@mrT23
Copy link
Collaborator

mrT23 commented Dec 1, 2023

@koid
I still dont understand this PR.
How AZURE_DEVOPS_CACHE_DIR is related to AWS lambda ?

Maybe it is a very specific use case, but it is not a general instruction for any AWS lambda .

Share a more logical and coherent instruction in order for it to be merged

@mrT23 mrT23 removed documentation Improvements or additions to documentation Review effort [1-5]: 1 labels Dec 1, 2023
@koid
Copy link
Contributor Author

koid commented Dec 5, 2023

This is not a specific use case but a common limitation in Lambda.

https://docs.aws.amazon.com/lambda/latest/dg/images-create.html#images-reqs

The container image must be able to run on a read-only file system. Your function code can access a writable /tmp directory with between 512 MB and 10,240 MB, in 1-MB increments, of storage.

As a prerequisite, in AWS Lambda, writable directories are restricted to those under /tmp. (Directories other than /tmp are treated as read-only.)

The pr-agent supports Azure DevOps as a git provider, but the Azure DevOps library requires write permissions to local storage for caching purposes.

When importing azure.devops.connection.Connection, the Connection class attempts to create a Cache directory. If the Cache directory is not in a writable area, an issue error will occur.

https://github.com/microsoft/azure-devops-python-api/blob/7.1.0b3/azure-devops/azure/devops/connection.py#L9
https://github.com/microsoft/azure-devops-python-api/blob/7.1.0b3/azure-devops/azure/devops/_file_cache.py#L114-L115

If the import of the Connection is done within the AzureDevopsProvider class, it is possible to avoid errors when not using the Azure DevOps provider. However, since the import is done outside the AzureDevopsProvider class, errors will still occur even if the Azure DevOps provider is not being used.

from azure.devops.connection import Connection

from pr_agent.git_providers.azuredevops_provider import AzureDevopsProvider

To avoid this, you need to specify AZURE_DEVOPS_CACHE_DIR to a writable directory (or you can specify it to /tmp, which is a writable directory under HOME) as a workaround.

--

Perhaps, in terms of chronology, Lambda support was added first (#92), followed by the Azure DevOps Provider support (#265), which might explain why the Lambda specifications were not taken into consideration during #265.

@mrT23
Copy link
Collaborator

mrT23 commented Dec 5, 2023

@koid
thanks for the detailed explanation

i will add a link to that explanation in the readme, since without it, it won't be clear how the things are connected

@mrT23 mrT23 merged commit 70a409a into Codium-ai:main Dec 5, 2023
1 check passed
@koid koid deleted the fix/specify-cache-directory-in-azure-devops branch December 6, 2023 01:25
yochail pushed a commit to yochail/pr-agent that referenced this pull request Feb 11, 2024
…y-in-azure-devops

Specify Cache Directory in Azure DevOps in Lambda Function Setup
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.

None yet

3 participants