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.

Sign up for GitHub

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

Jump to bottom

chore: Simplifies makefile #2003

Merged
merged 3 commits into from
Mar 11, 2024
Merged

chore: Simplifies makefile #2003

merged 3 commits into from
Mar 11, 2024

Conversation

lantoli
Copy link
Member

@lantoli lantoli commented Mar 11, 2024 •
edited
Loading

Description

Simplifies makefile now that there are not special folders like integrationtesting.

Type of change:

  • Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
  • New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR.
  • Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR.
  • This change requires a documentation update
  • Documentation fix/enhancement

Required Checklist:

  • I have signed the MongoDB CLA
  • I have read the contribution guidelines
  • I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
  • I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
  • I have added any necessary documentation (if appropriate)
  • I have run make fmt and formatted my code
  • If changes include deprecations or removals, I defined an isolated PR with a relevant title as it will be used in the auto-generated changelog.
  • If changes include removal or addition of 3rd party GitHub actions, I updated our internal document. Reach out to the APIx Integration slack channel to get access to the internal document.

Further comments

@lantoli lantoli requested a review from a team as a code owner March 11, 2024 06:55
AgustinBettati
AgustinBettati approved these changes Mar 11, 2024
View reviewed changes
Copy link
Member

@AgustinBettati AgustinBettati left a comment

Choose a reason for hiding this comment

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

LGTM

GNUmakefile
Comment on lines -38 to +34
go test $(TEST) -timeout=30s -parallel=4 -race -covermode=atomic -coverprofile=coverage.out
go test ./... -timeout=30s -parallel=4 -race
Copy link
Member

Choose a reason for hiding this comment

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

just for reference I see this was included originally for in #1496, but we no longer include these converage reports as PR comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, at the moment we don't include any coverage report

.github/workflows/acceptance-tests-runner.yml Outdated
@@ -96,7 +96,6 @@ on:
required: true

env:
TF_ACC: 1
Copy link
Member

Choose a reason for hiding this comment

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

while redundant I do see the value in leaving the TF_ACC env var just for clarity

Copy link
Member Author

Choose a reason for hiding this comment

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

don't know, if we assume "make testacc" does it i would remove it from here.

i would choose only one place to have it

Copy link
Collaborator

@oarbusi oarbusi Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

[q] Does this mean that TF_ACC is not needed to run acceptance tests? Including locally? If so, we should update Terraform Development Guide

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 I see the value in keeping it

Copy link
Member Author

Choose a reason for hiding this comment

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

@oarbusi testacc makefile goal is: TF_ACC=1 go test ...

Copy link
Member Author

Choose a reason for hiding this comment

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

change reverted, i'll keep it, thx for the feedback

andreaangiolillo
andreaangiolillo reviewed Mar 11, 2024
View reviewed changes
GNUmakefile
Comment on lines -84 to -85
.PHONY: check
check: test lint
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we remove this? I would include additional check to this command such as fmtcheck

Copy link
Member Author

@lantoli lantoli Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

it's not being used in GH actions or commit prechecks and i don't think anybody is using this

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah okay, I was probably the only one using it xD happy to remove it then

.github/workflows/acceptance-tests-runner.yml Outdated
@@ -96,7 +96,6 @@ on:
required: true

env:
TF_ACC: 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 I see the value in keeping it

@lantoli
Copy link
Member Author

lantoli commented Mar 11, 2024

changing only makefile

@lantoli lantoli merged commit 0e99328 into master Mar 11, 2024
45 checks passed
@lantoli lantoli deleted the makefile_simplify branch March 11, 2024 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreaangiolillo andreaangiolillo andreaangiolillo left review comments

@oarbusi oarbusi oarbusi left review comments

@AgustinBettati AgustinBettati AgustinBettati approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lantoli @andreaangiolillo @AgustinBettati @oarbusi