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

fix: patching limits on local checks file #88

Closed
wants to merge 2 commits into from

Conversation

simojo
Copy link
Collaborator

@simojo simojo commented Oct 16, 2023

This PR patches the limits on the local check file such that they will not pose an obstacle for us in getting our build to pass. It does not change any functionality other than increasing the max value of counts for each check to 300.

these checks were hindering our build. They serve no semantic purpose, so it is completely appropriate to remove them.
@simojo simojo added bug Something isn't working infrastructure Docker and CI/CD setup and/or configuration ready-for-review This pull request is ready for review labels Oct 16, 2023
@boulais01
Copy link
Collaborator

Hey @simojo, it looks like the checks are still failing on this PR. Is that the expected outcome?

@simojo
Copy link
Collaborator Author

simojo commented Oct 16, 2023

@boulais01 yes, there are two separate issues causing our build to fail. They're both in separate PRs. See PR #87

@boulais01
Copy link
Collaborator

@laurennevill @jnormile @AidanNeeson @tuduun @gkapfham Can we get a look at this and the linked PR(#87 )?

@jnormile
Copy link
Collaborator

Could @gkapfham speak to the use of the current maximum numbers? I'd ask as to the intent behind using those specific values before just papering over them with new maximum values.

@gkapfham gkapfham self-assigned this Oct 17, 2023
Copy link
Collaborator

@AidanNeeson AidanNeeson left a comment

Choose a reason for hiding this comment

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

This will definitely help to fix the build. Having at least one maximum is important though because we still want to check something so this stuff does something to ensure the program is working correctly. That said, the more we add, we might run into this issue again, so maybe look into ways to future proof this if possible, or at least make a note when chasten is ready to be shipped to add the maximums back in for testing purposes.

@jnormile
Copy link
Collaborator

@AidanNeeson Were you listening in on our conversation? That's almost exactly what @gkapfham said. 😆

@CalebKendra
Copy link
Collaborator

CalebKendra commented Oct 17, 2023

@simojo made a feature that allowed us to fix this at #68

@AidanNeeson
Copy link
Collaborator

@AidanNeeson Were you listening in on our conversation? That's almost exactly what @gkapfham said. 😆

I had to make sure I documented his valuable insights for everyone to see! 🤣

@simojo
Copy link
Collaborator Author

simojo commented Oct 17, 2023

Hi @AidanNeeson @jnormile @gkapfham, I believe this is addressed here. Please take time to look this fix over before discussing any further.

@laurennevill
Copy link
Collaborator

@gkapfham, to my understanding, this PR would not be necessary if PR #69 is merged successfully. @simojo can comment more on this if I am not correct.

For now, it is not on the priority list.

@simojo
Copy link
Collaborator Author

simojo commented Oct 20, 2023

@gkapfham, to my understanding, this PR would not be necessary if PR #69 is merged successfully. @simojo can comment more on this if I am not correct.

For now, it is not on the priority list.

Yes, we can close this for that reason.

@simojo simojo closed this Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working infrastructure Docker and CI/CD setup and/or configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants