Skip to content
This repository has been archived by the owner on Jan 8, 2024. It is now read-only.

Reintegrate skeleton #126

Closed
wants to merge 388 commits into from
Closed

Reintegrate skeleton #126

wants to merge 388 commits into from

Conversation

zenine07
Copy link
Contributor

@zenine07 zenine07 commented May 11, 2021

🗣 Description

This PR would reintegrate the cisagov/skeleton-docker skeleton into this repository.

This was done by adding the correct skeleton repository as a remote, pulling from its develop branch, and resolving any conflicts shown.

git remote add skeleton https://github.com/cisagov/skeleton-docker.git
git remote set-url --push skeleton no_push
git pull --allow-unrelated-histories skeleton develop
git status

After skeleton was merged, fixed pre-commit issues that showed up.

💭 Motivation and Context

When this project was created, it was not started using the skeleton tool This prevents the lineage GitHub Action from automatically generating PRs to ensure downstream repositories are kept up-to-date.

🧪 Testing

Automated tests pass.

🚥 Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (causes existing functionality to change)
  • Integrate skeleton

✅ Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

felddy and others added 30 commits November 5, 2019 15:54
Add GitHub action caching of pre-commit hooks and pip packages.
Migrate CI system and Docker Hub organization
Add missing env variable for test
…thon

Set a default language for pre-commit hooks
Add codeowners file with team OIS maintainers.
A user forked cisagov/scan-target-data and created a pull request, but
the required GitHub Action(s) did not run.  This is presumably because
the user does not have Actions enabled in his or her fork.  Ideally,
the required Action(s) would run in cisagov/scan-target-data when a PR
to merge changes back is created.  Based on my reading of this link,
adding the "pull_request" event type should make this happen:
https://help.github.com/en/actions/automating-your-workflow-with-github-actions/events-that-trigger-workflows#pull-request-events-for-forked-repositories
…m-forked-repos

Make workflow run when a PR is opened, synchronized, or reopened
@zenine07 zenine07 self-assigned this May 11, 2021
@zenine07 zenine07 requested a review from dav3r May 12, 2021 13:08
@zenine07
Copy link
Contributor Author

@dav3r Are we able to disable the LGTM analysis: Python on this repo?

@dav3r
Copy link
Member

dav3r commented May 12, 2021

@dav3r Are we able to disable the LGTM analysis: Python on this repo?

Yes, I will disable it for now. Can you please remind me to re-enable it after this PR is merged?

@zenine07
Copy link
Contributor Author

Yes, I will disable it for now. Can you please remind me to re-enable it after this PR is merged?

I can, but why does it need to be re-enabled when this isn't a Python project?

@dav3r
Copy link
Member

dav3r commented May 12, 2021

@dav3r Are we able to disable the LGTM analysis: Python on this repo?

Yes, I will disable it for now. Can you please remind me to re-enable it after this PR is merged?

I just checked and none of the checks are required for merging yet, so don't worry about LGTM failing for now.

@zenine07 zenine07 marked this pull request as ready for review May 12, 2021 14:07
@zenine07 zenine07 requested a review from inlguy as a code owner May 12, 2021 14:07
@zenine07 zenine07 requested a review from jsf9k May 12, 2021 14:07
@dav3r
Copy link
Member

dav3r commented May 12, 2021

Yes, I will disable it for now. Can you please remind me to re-enable it after this PR is merged?

I can, but why does it need to be re-enabled when this isn't a Python project?

In this case, LGTM will still be checking the requirements files and the Python code in the tests/ directory. It's not much, but it doesn't hurt to have it run.

@zenine07
Copy link
Contributor Author

In this case, LGTM will still be checking the requirements files and the Python code in the tests/ directory. It's not much, but it doesn't hurt to have it run.

Sounds good. I'll see if I can get it to succeed.

@dav3r
Copy link
Member

dav3r commented May 12, 2021

In this case, LGTM will still be checking the requirements files and the Python code in the tests/ directory. It's not much, but it doesn't hurt to have it run.

Sounds good. I'll see if I can get it to succeed.

If you can't, don't sweat it too much on this PR. We have seen it behave in weird ways when it is initially integrated to a project, but tends to work after that.

Copy link
Member

@jsf9k jsf9k left a comment

Choose a reason for hiding this comment

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

Great work @zenine07! I requested only a couple of changes.

@@ -0,0 +1 @@
There are no secrets better kept than the secrets everybody guesses.
Copy link
Member

Choose a reason for hiding this comment

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

Is this file being used here? If not then it should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsf9k I forget that file every time. Fixed in 26d7bb2

README.md Outdated
@@ -70,6 +70,6 @@ Remove containers:

Angular app located at [localhost:4200](http://localhost:4200)

### Run pre-commit:
### Run pre-commit
Copy link
Member

Choose a reason for hiding this comment

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

We generally do Markdown headings ## Like this ## with trailing octothorpes. Can you make this change throughout this file?

I'm also confused as to why the linters aren't checking this file. @mcdonnnj?

Copy link
Member

@mcdonnnj mcdonnnj May 12, 2021

Choose a reason for hiding this comment

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

We don't have anything auto-formatting on Markdown files. We would need to expand the markdownlint configuration to enforce this rule because it is on the default consistent. We should really look at adding a number of rules to the markdownlint configuration to better enforce our preferred Markdown style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mcdonnnj I believe it was automatically enforcing it when I ran precommit for the python-library-skeleton, but it doesn't do it in the skeleton-docker.

Copy link
Member

Choose a reason for hiding this comment

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

@zenine07 The default of consistent will only flag if you are using different styles. Since you did not merge in the skeleton's README you are consistently using the ATX style (and not the ATX closed style we prefer).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsf9k Made those changes in 9855810

@zenine07 zenine07 closed this Jun 9, 2021
@zenine07 zenine07 deleted the reintegrate_skeleton branch June 9, 2021 12:36
@zenine07 zenine07 restored the reintegrate_skeleton branch June 9, 2021 12:37
cisagovbot pushed a commit that referenced this pull request Jul 14, 2023
…nd-wheel-with-pip

Install/upgrade setuptools and wheel when upgrading pip
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants