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

Add the flask-framework extension #1582

Merged
merged 2 commits into from
Mar 21, 2024
Merged

Add the flask-framework extension #1582

merged 2 commits into from
Mar 21, 2024

Conversation

weiiwang01
Copy link
Contributor

Bring the flask-framework extension from the feature/12f branch into the main branch.

The flask-extension is a part of the 12-factor toolchain that assists users in creating a charm to manage their Flask application within the Juju ecosystem.

As we are still awaiting some changes in other libraries to be finalized, please review the current content. I will update the pull request once the changes in the other libraries have been merged.

@weiiwang01
Copy link
Contributor Author

@lengau, here's another pull request that cherry-picks only the flask-framework into the main branch, as per @sergiusens's recommendation.

@lengau lengau requested review from lengau and syu-w March 6, 2024 19:02
Copy link
Collaborator

@lengau lengau left a comment

Choose a reason for hiding this comment

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

Thanks for this!

Looking back at the other PR, I wasn't clear in what I was asking for with reorganising the constants for the gunicorn extensions. Sorry about that!

I've made what should be a clearer example of the change I'm requesting.

charmcraft/application/main.py Outdated Show resolved Hide resolved
charmcraft/extensions/__init__.py Outdated Show resolved Hide resolved
charmcraft/extensions/gunicorn.py Outdated Show resolved Hide resolved
@@ -101,6 +101,7 @@ def create_actions_yaml(
shutil.copyfile(original_file_path, target_file_path)
else:
if charmcraft_config.actions:
basedir.mkdir(exist_ok=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I gather you added this because something was failing - can you let me know what was failing, as that's probably a sign of a bigger bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, this is too distant in the past, and I can't remember the context behind this change, and workflow logs have all been lost. Judging by this line, it seems to be caused by the prime directory not existing when creating the action.yaml?

Let me remove this line and run the test to see if that can trigger an error.

tests/spread/commands/init-flask-framework/task.yaml Outdated Show resolved Hide resolved
charmcraft/extensions/gunicorn.py Outdated Show resolved Hide resolved
charmcraft/application/commands/init.py Show resolved Hide resolved
@lengau lengau self-requested a review March 7, 2024 21:00
Copy link
Collaborator

@lengau lengau left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

It looks like you have a different version of black from what we're using in CI. Running tox -f format with a recent enough version of tox (3.8+, so you should be ok to install tox from apt all the way back to jammy) should be consistent.

charmcraft/extensions/gunicorn.py Show resolved Hide resolved
@lengau
Copy link
Collaborator

lengau commented Mar 18, 2024

@weiiwang01 I'm not sure why this PR is causing the tests to timeout on macos, but it seems like it's waiting for user input, I think when running the integration tests for the init command. Could you check the permissions on the files you added, especially the templates?

@weiiwang01
Copy link
Contributor Author

@weiiwang01 I'm not sure why this PR is causing the tests to timeout on macos, but it seems like it's waiting for user input, I think when running the integration tests for the init command. Could you check the permissions on the files you added, especially the templates?

I check the init templates, and the file permissions should be consistent with the other templates. Let me temporarily change the test command to see if we can identify which test is blocking.

@weiiwang01
Copy link
Contributor Author

@weiiwang01 I'm not sure why this PR is causing the tests to timeout on macos, but it seems like it's waiting for user input, I think when running the integration tests for the init command. Could you check the permissions on the files you added, especially the templates?

I check the init templates, and the file permissions should be consistent with the other templates. Let me temporarily change the test command to see if we can identify which test is blocking.

Okay, I understand the problem: the flask-framework init profile does not generate a tox.ini file (in that use case, the tox configuration should be located in the project root, not in the charm/ directory). So, the test_tox_success test ends up executing the tox environment in charmcraft, which causes an infinite loop. I have added a directive to skip the test_tox_success test if a tox.ini file is not generated.

@lengau lengau merged commit c060832 into canonical:main Mar 21, 2024
15 checks passed
@weiiwang01 weiiwang01 deleted the add-flask-framework branch March 21, 2024 16:24
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