Skip to content
This repository has been archived by the owner on Sep 22, 2023. It is now read-only.

First commits #1

Merged
merged 30 commits into from
Jan 27, 2023
Merged

First commits #1

merged 30 commits into from
Jan 27, 2023

Conversation

itsmostafa
Copy link
Contributor

@itsmostafa itsmostafa commented Jan 18, 2023

Initialize Con-PCA Tasks utilizing Golang

🗣 Description

  • Added Golang pre-commits
  • Initialized repository with a base Golang project
  • Updated Readme and Gitignore files
  • Add Chi Router dependancy

Note: I placed the golang script in the root of the project instead of in an src folder due to golang project layout standards:
https://github.com/golang-standards/project-layout#src

💭 Motivation and context

To build a scalable, more robust version of the con-pca's task scheduler.

🧪 Testing

ran go test -v ./... or make test

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated
    to reflect the changes in this PR.
  • Tests have been added and/or modified to cover the changes in this PR.
  • All new and existing tests pass.

✅ Pre-merge checklist

  • Revert dependencies to default branches.
  • Finalize version.

✅ Post-merge checklist

  • Add a tag or create a release.

@itsmostafa itsmostafa marked this pull request as ready for review January 18, 2023 05:43
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.

  • What is the file go.sum? Does it need to be included?
  • That eight space tab setting is pretty insane. A few levels of indentation and you'll be hitting the right hand margin. Is that necessary? Four spaces is more standard, but maybe eight spaces is a go thing.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
@jsf9k
Copy link
Member

jsf9k commented Jan 18, 2023

I unclicked the checkbox titled "Tests have been added and/or modified to cover the changes in this PR" since I don't see any tests here. If checkboxes do not apply they can be deleted, but they should not be checked if they are not satisfied.

@itsmostafa
Copy link
Contributor Author

  • What is the file go.sum? Does it need to be included?
  • That eight space tab setting is pretty insane. A few levels of indentation and you'll be hitting the right hand margin. Is that necessary? Four spaces is more standard, but maybe eight spaces is a go thing.

@jsf9k yes go.sum needs to be included as go is expecting it. it's just empty at the moment because I don't have any modules installed to the project yet. think of it as similar to Pipfile.lock in python or package-lock.json in JS.

Which file do you see the 8 spaces tab?

@itsmostafa
Copy link
Contributor Author

I unclicked the checkbox titled "Tests have been added and/or modified to cover the changes in this PR" since I don't see any tests here. If checkboxes do not apply they can be deleted, but they should not be checked if they are not satisfied.

@jsf9k i actually do have a test i added. It's in controllers/controllers_test.go. It's common practice in golang for test files to exist next to the source code you are testing. You would just append _test.go to the end of it. I'm aware its different from Python where all unit tests live in its own tests directory.

@jsf9k
Copy link
Member

jsf9k commented Jan 18, 2023

I unclicked the checkbox titled "Tests have been added and/or modified to cover the changes in this PR" since I don't see any tests here. If checkboxes do not apply they can be deleted, but they should not be checked if they are not satisfied.

@jsf9k i actually do have a test i added. It's in controllers/controllers_test.go. It's common practice in golang for test files to exist next to the source code you are testing. You would just append _test.go to the end of it. I'm aware its different from Python where all unit tests live in its own tests directory.

I have no idea how I missed that. Sorry!

Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

I noted a few items to get you started:

.github/CODEOWNERS Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@jsf9k
Copy link
Member

jsf9k commented Jan 18, 2023

  • What is the file go.sum? Does it need to be included?
  • That eight space tab setting is pretty insane. A few levels of indentation and you'll be hitting the right hand margin. Is that necessary? Four spaces is more standard, but maybe eight spaces is a go thing.

@jsf9k yes go.sum needs to be included as go is expecting it. it's just empty at the moment because I don't have any modules installed to the project yet. think of it as similar to Pipfile.lock in python or package-lock.json in JS.

Which file do you see the 8 spaces tab?

All the *.go files, as well as the Makefile.

@itsmostafa
Copy link
Contributor Author

  • What is the file go.sum? Does it need to be included?
  • That eight space tab setting is pretty insane. A few levels of indentation and you'll be hitting the right hand margin. Is that necessary? Four spaces is more standard, but maybe eight spaces is a go thing.

@jsf9k yes go.sum needs to be included as go is expecting it. it's just empty at the moment because I don't have any modules installed to the project yet. think of it as similar to Pipfile.lock in python or package-lock.json in JS.
Which file do you see the 8 spaces tab?

All the *.go files, as well as the Makefile.

@jsf9k it looks like my local tab indentations are translating to 8 spaces in Github for some reason (They actually look like 4 in VSCode locally). I just pushed a commit where i used spaces instead in the Makefile: 9397e89. I will continue to investigate. But yes I agree 8 is way too much.

@itsmostafa
Copy link
Contributor Author

itsmostafa commented Jan 18, 2023

@jsf9k

  • What is the file go.sum? Does it need to be included?
  • That eight space tab setting is pretty insane. A few levels of indentation and you'll be hitting the right hand margin. Is that necessary? Four spaces is more standard, but maybe eight spaces is a go thing.

@jsf9k yes go.sum needs to be included as go is expecting it. it's just empty at the moment because I don't have any modules installed to the project yet. think of it as similar to Pipfile.lock in python or package-lock.json in JS.
Which file do you see the 8 spaces tab?

All the *.go files, as well as the Makefile.

@jsf9k it looks like my local tab indentations are translating to 8 spaces in Github for some reason (They actually look like 4 in VSCode locally). I just pushed a commit where i used spaces instead in the Makefile: 9397e89. I will continue to investigate. But yes I agree 8 is way too much.

@jsf9k after doing more research, it looks like the default for tab size in Github is set to 8. if you append ?ts=4 to a github file URL, it would go down to 4 spaces like so: https://github.com/cisagov/con-pca-tasks/blob/9397e896b2e9d28a6d7445fac40da4d44a09b0bb/controllers/controllers_test.go?ts=4

As previously mentioned, locally on my code editor, it looks like 4 spaces.

I did some research and found this: https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-personal-account-settings/managing-your-tab-size-rendering-preference

I couldn't find a way to manage tab size at the repo level but you can update it at a user level. however it'll visually update all tab sizes in all repos that you view

@jsf9k
Copy link
Member

jsf9k commented Jan 18, 2023

@jsf9k after doing more research, it looks like the default for tab size in Github is set to 8. if you append ?ts=4 to a github file URL, it would go down to 4 spaces like so: https://github.com/cisagov/con-pca-tasks/blob/9397e896b2e9d28a6d7445fac40da4d44a09b0bb/controllers/controllers_test.go?ts=4

You should be using spaces, not tab characters. You should be able to tell your editor to interpret a tab keypress as four spaces.

@itsmostafa
Copy link
Contributor Author

itsmostafa commented Jan 18, 2023

@jsf9k after doing more research, it looks like the default for tab size in Github is set to 8. if you append ?ts=4 to a github file URL, it would go down to 4 spaces like so: https://github.com/cisagov/con-pca-tasks/blob/9397e896b2e9d28a6d7445fac40da4d44a09b0bb/controllers/controllers_test.go?ts=4

You should be using spaces, not tab characters. You should be able to tell your editor to interpret a tab keypress as four spaces.

@jsf9k I'm using the golang standard gofmt to format my project which is strongly recommended. I tried looking for a way i can get the formatter to use spaces instead of indentations. gofmt is very opinionated and doesn't have flexible configuration. If I try to use spaces, the formatter will automatically replace them with indentations. They used to have a -tabwidth flag but now that has been removed

After some research, tabs for indentation is the official Golang recommendation:
See here and here

Please let me know your thoughts and how you'd like me to move forward. Thanks!

@jsf9k
Copy link
Member

jsf9k commented Jan 18, 2023

After some research, tabs for indentation is the official Golang recommendation: See here and here

Please let me know your thoughts and how you'd like me to move forward. Thanks!

Go ahead and let gofmt do what it wants with the Go source files, but make the Makefile use four spaces instead of tab characters. (This will match what we do in our other repos.)

Interesting that Go has decided to re-ignite this conflict.

@itsmostafa
Copy link
Contributor Author

@itsmostafa You should modify the branch protection rules in this repo so that you cannot merge until all conversations have been resolved. That is our standard setup.

done!

Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

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

Please add an appropriate CodeQL configuration. You can crib the cisagov/skeleton-python-library one and just update the language configuration appropriately (and optionally change the cron configuration to avoid overlap).

@itsmostafa
Copy link
Contributor Author

Please add an appropriate CodeQL configuration. You can crib the cisagov/skeleton-python-library one and just update the language configuration appropriately (and optionally change the cron configuration to avoid overlap).

@mcdonnnj done!

Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

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

Please add a test job to the GitHub Actions workflow. I see that there is make test functionality so I imagine it should be straightforward to add something similar to the build workflow much like in the Python skeleton. If you feel like this is too much please create an issue to log this as future work but I strongly recommend this work gets done in the near future to provide good guardrails for pull requests.

@itsmostafa
Copy link
Contributor Author

Please add a test job to the GitHub Actions workflow. I see that there is make test functionality so I imagine it should be straightforward to add something similar to the build workflow much like in the Python skeleton. If you feel like this is too much please create an issue to log this as future work but I strongly recommend this work gets done in the near future to provide good guardrails for pull requests.

@mcdonnnj resolved with f0ce5c7 & cd17276

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@itsmostafa itsmostafa force-pushed the first-commits branch 2 times, most recently from 443b4e5 to c2c2e36 Compare January 26, 2023 17:12
Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

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

This looks pretty solid. I think it would be helpful to make a build job in the GitHub Actions workflow that creates a binary and uploads that as an artifact, but that is not strictly necessary right now. Maybe make an issue to track that as future work?

@itsmostafa
Copy link
Contributor Author

This looks pretty solid. I think it would be helpful to make a build job in the GitHub Actions workflow that creates a binary and uploads that as an artifact, but that is not strictly necessary right now. Maybe make an issue to track that as future work?

Awesome, Sounds good. Yes I was planning on working on that effort in the near future. I'll create an issue.

@itsmostafa itsmostafa merged commit 9b2b18a into develop Jan 27, 2023
@itsmostafa itsmostafa deleted the first-commits branch January 27, 2023 17:24
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.

5 participants