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

aac-puml project infrastructure setup (#774) #1

Merged
merged 76 commits into from
Mar 13, 2024
Merged

aac-puml project infrastructure setup (#774) #1

merged 76 commits into from
Mar 13, 2024

Conversation

crazynewidea
Copy link
Contributor

intermediate pull request to test main branch push workflow

.github/ISSUE_TEMPLATE/SIMPLE-TASK.yml Outdated Show resolved Hide resolved
.github/workflows/python-cross-platform-support.yml Outdated Show resolved Hide resolved
.github/workflows/sphinx.yml Outdated Show resolved Hide resolved
.github/workflows/sphinx.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/CNAME Outdated Show resolved Hide resolved
.gitpod.yml Outdated Show resolved Hide resolved
docs/source/conf.py Outdated Show resolved Hide resolved
docs/source/images/user_library/inactive_plugins.png Outdated Show resolved Hide resolved
docs/source/images/user_library/list_active.png Outdated Show resolved Hide resolved
docs/source/images/user_library/revalidate_example.png Outdated Show resolved Hide resolved
docs/source/images/user_library/validate_example.png Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

So, I don't think it would get to go away fully because we still need to bundle the secure install components into the zip since that is something that gets provided for project hosting. But the steps that were previously producing the hash output that is now able to be accomplished with the build tool changes should get to be altered.

I believe it is outside of the scope of this task to fully determine what would have to change and how. Believe it is a solid candidate for a few tasks within the future pipeline refinement and buildtool modernization features.

@lizzcondrey
Copy link
Contributor

Working through Will's aac-spec PR found the python-deploy-artifacts.yml workflow that reminded me that I forgot to put a comment to your PR to include. But with a note of commenting out lines 5-9 about the push to main.
Need to comment the push to main portion out so that it does not try to automatically deploy to PyPi yet since we do not have a functioning stand-alone package yet. We should only be manually deploying till we know it is fully functioning and stable and safe to put to automatic deployments.
Also, we need to confirm we have a registration with PyPi for the package.

The deployment of artifacts happens in the python-build-and-lint.yml worflow in the upload-artifacts action. Agree we need to confirm registration with PyPi but seems out of scope for this code review.

Partially correct, while an upload-artifacts does occur in the python-build-and-lint.yml workflow, that is not an upload to PyPi. That is handled by the python-deploy-artifacts.yml workflow as mentioned in my original comment.

Agree confirming the registration with PyPi is outside the scope of this task and code review, was just including it for SA.

Copy link
Contributor

@lizzcondrey lizzcondrey left a comment

Choose a reason for hiding this comment

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

There were some change suggestions missed, tagged those with new comments. And flagged files that are still needing to be removed. Good job getting through initial round of comments.

@lizzcondrey
Copy link
Contributor

I don't see any .gitignore files for your PR, need to make sure the documentation one is pulled in as well as the src code level one.
Also don't see the .pylintrc file included which would be used with the linting step in the pipeline, but you removed that step. Both need to be included.

.gitignore is at the root of the project directory. .pylintrc added as requested additional .gitingnore added in docs folder as requested

The .gitignore at root was from the original initial commit of standing up the repository, it won't have any of the additional AaC ignore additions. Did you make changes to it prior to moving to the dev branch to update with the additional ignore additions? If so, it wouldn't have been grabbed for this PR so didn't have a way to review if it included the additional AaC ignores.

Copy link
Contributor

@lizzcondrey lizzcondrey left a comment

Choose a reason for hiding this comment

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

Good job getting version tags reassigned everywhere you had main, but it looks like you migrated versions and that can cause a lot of breakage with the actions within the workflows. Make sure you are using what is currently in core, we have an upcoming pipeline improvement feature that would tackle migrating action versions since that is well outside of the scope of the infrastructure task and the PUML feature.

.github/ISSUE_TEMPLATE/SIMPLE-TASK.yml Outdated Show resolved Hide resolved
.github/workflows/codeql-analysis.yml.disabled Outdated Show resolved Hide resolved
.github/workflows/python-cross-platform-support.yml Outdated Show resolved Hide resolved
.github/workflows/python-build-and-lint.yml Show resolved Hide resolved
.github/workflows/sphinx.yml Show resolved Hide resolved
docs/source/conf.py Outdated Show resolved Hide resolved
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

Copy link
Contributor

@lizzcondrey lizzcondrey left a comment

Choose a reason for hiding this comment

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

Think this will be the last round. Looks like the action versions all got corrected, trusting the tweaks to the toml file being a result of something you're seeing since it is a new approach, and Gitpod launching seems to have been fixed since I can get it to launch now, and we were having issues with it yesterday.

Comment on lines +6 to +13
# # Python Dependencies
# ARG PYTHON_VERSION=3.9.13

# RUN sudo add-apt-repository ppa:deadsnakes/ppa -y
# RUN sudo apt install python${PYTHON_VERSION} -y
# RUN sudo apt install python${PYTHON_VERSION}-venv -y
# RUN sudo update-alternatives --install /usr/bin/python3 python3 /usr/bin/python${PYTHON_VERSION} 2
# RUN sudo pip install --upgrade pip
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did these have to be commented out?

Copy link
Contributor Author

@crazynewidea crazynewidea Mar 13, 2024

Choose a reason for hiding this comment

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

they were causing the gitpod load to fail. This change made the gitpod load quicker and still accomplishes the python version load that we want. I left the commented out code there for reference in case we still wanted any of it. Is there something there you want to revive?

Copy link
Contributor

Choose a reason for hiding this comment

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

Curious about why this was causing issues for you but not with the aac-spec repo. As long as you're tracking it as an issue and this is currently working, we can evaluate it as we continue to work in the repository and maintain what a rollback would look like with the commented section. This should be okay for now.

Copy link
Contributor

@lizzcondrey lizzcondrey left a comment

Choose a reason for hiding this comment

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

I missed catching some of the upload-artifact versions not getting corrected to v3.

.github/workflows/python-cross-platform-support.yml Outdated Show resolved Hide resolved
.github/workflows/python-cross-platform-support.yml Outdated Show resolved Hide resolved
.github/workflows/python-test-python-version.yml Outdated Show resolved Hide resolved
.github/workflows/python-test-python-version.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@lizzcondrey lizzcondrey left a comment

Choose a reason for hiding this comment

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

Good job getting everything addressed and incorporated. Identified some good spots for further evaluation and improvement within our upcoming pipeline feature.

@crazynewidea crazynewidea merged commit 1568a12 into main Mar 13, 2024
7 of 12 checks passed
@crazynewidea crazynewidea deleted the dev branch March 29, 2024 04:54
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