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

Check MANIFEST when PR is created #195

Merged
merged 2 commits into from
Oct 15, 2024
Merged

Check MANIFEST when PR is created #195

merged 2 commits into from
Oct 15, 2024

Conversation

MichaelTimbert
Copy link
Contributor

@MichaelTimbert MichaelTimbert commented Aug 19, 2024 •
edited
Loading

Purpose

Run 'make distcheck' in tests.
I followed what was done in "zonemaster/zonemaster-engine#903"
except that I added "t/manifest.t" file in MANIFEST.SKIP otherwise the installation of tar.gz fails.

Context

Fixes #119

How to test this PR

Make sure we don't interfere with the release process:

git clean -dfx   # Start out with a clean repository
perl Makefile.PL # Generate the top-level Makefile
make all         # Generate MO files and modules.txt
make distcheck   # Verify that distcheck doesn't report any missing or unrecognized files

Run the unit tests in the source directory:

make distcheck     # Make sure distcheck doesn't report any missing or unrecognized files
prove t/manifest.t # Verify the new test passes
touch new.file  # Create a file but don't add it to MANIFEST
prove t/manifest.t # Verify the new test fails
echo "new.file" >> MANIFEST.SKIP 
prove t/manifest.t # Verify the new test passes

Run the unit tests in the dist file:

make distcheck                               # Make sure your repo is clean
make dist                                    # Create the dist file
cpanm --test-only Zonemaster-LDNS-*.tar.gz # Run the tests included in the dist file

@MichaelTimbert MichaelTimbert added the S-PRforIssue Status: There is a PR that is meant to resolve the issue label Aug 19, 2024
@MichaelTimbert MichaelTimbert added this to the v2024.2 milestone Aug 19, 2024
matsduf
matsduf reviewed Aug 19, 2024
View reviewed changes
Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

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

The only other repository that has t/manifest.t is Zonemaster-Engine (I think that -CLI and -Backend also should have it). I made a comparison between this t/manifest.t and the one from Zonemaster-Engine and they are not 100% identical. Is there any reason for that?

Here t/manifest.t is in the MANIFEST.SKIP file, which seams correct, but in Zonemaster-Engine it is in MANIFEST.

I think we should have the same handling in all the repositories, if possible.

@MichaelTimbert MichaelTimbert self-assigned this Aug 20, 2024
@MichaelTimbert
Copy link
Contributor Author

The only other repository that has t/manifest.t is Zonemaster-Engine (I think that -CLI and -Backend also should have it). I made a comparison between this t/manifest.t and the one from Zonemaster-Engine and they are not 100% identical. Is there any reason for that?

No, i have updated 't/manifest.t" to be the same as the on on Zonemaster-Engine.

@matsduf
Copy link
Contributor

matsduf commented Aug 20, 2024

Run the unit tests in the dist file:

make distcheck                               # Make sure your repo is clean
make dist                                    # Create the dist file
cpanm --test-only Zonemaster-LDNS-*.tar.gz # Run the tests included in the dist file

Since t/manifest.t is not in MANIFEST it will not be included in the dist file, and nothing special will happen with cpanm --test-only. It is more relevant to see what how Travis handles updated and not updated MANIFEST and MANIFEST.SKIP files.

matsduf
matsduf previously approved these changes Aug 20, 2024
View reviewed changes
Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

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

Works as expected in Travis.

tgreenx
tgreenx previously approved these changes Aug 20, 2024
View reviewed changes
@matsduf
Copy link
Contributor

matsduf commented Aug 20, 2024

@MichaelTimbert, do you plan to update Zonemaster-Engine and copy this PR to Zonemaster-CLI and Zonemaster-Backend to make all have the same behavior?

@MichaelTimbert MichaelTimbert mentioned this pull request Aug 21, 2024
Merged
@MichaelTimbert
Copy link
Contributor Author

I'm not sure about this solution, I read @mattias-p's comments on zonemaster/zonemaster-engine#903 and agree that this is more like a hack than a correct solution.
To me /t contains functional tests and that's what people expect. Adding the MANIFEST check is more of a Continuous Delivery(CD) test because it check the tar.gz file and not the code itself.
This is why i think it preferable to test MANIFEST* in github action (as proposed in #201), we can separate CI and CD by checking MANIFEST in another github-action's job for more readability. But in this case we will be more dependent of Github for testing.

@matsduf
Copy link
Contributor

matsduf commented Aug 22, 2024

This is why i think it preferable to test MANIFEST* in github action (as proposed in #201), we can separate CI and CD by checking MANIFEST in another github-action's job for more readability. But in this case we will be more dependent of Github for testing.

We host Zonemaster on Github. Is the dependency on Github for CI work really a problem?

@MichaelTimbert
Copy link
Contributor Author

We host Zonemaster on Github. Is the dependency on Github for CI work really a problem?

No, not really , it is just something to get in mind while making this choice.

@MichaelTimbert
Copy link
Contributor Author

@MichaelTimbert, do you plan to update Zonemaster-Engine and copy this PR to Zonemaster-CLI and Zonemaster-Backend to make all have the same behavior?

Yes i will do it :)

This was referenced Aug 27, 2024
Merged
Merged
@MichaelTimbert MichaelTimbert changed the base branch from master to develop August 28, 2024 06:47
@MichaelTimbert MichaelTimbert dismissed stale reviews from tgreenx and matsduf August 28, 2024 06:47

The base branch was changed.

@MichaelTimbert MichaelTimbert mentioned this pull request Aug 29, 2024
Merged
@matsduf matsduf removed the S-PRforIssue Status: There is a PR that is meant to resolve the issue label Sep 4, 2024
@matsduf matsduf added the V-Patch Versioning: The change gives an update of patch in version. label Oct 11, 2024
@matsduf
Copy link
Contributor

matsduf commented Oct 11, 2024

@MichaelTimbert, please merge.

@MichaelTimbert MichaelTimbert merged commit f28e22a into zonemaster:develop Oct 15, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marc-vanderwal marc-vanderwal marc-vanderwal approved these changes

@matsduf matsduf matsduf approved these changes

@tgreenx tgreenx tgreenx left review comments

@mattias-p mattias-p Awaiting requested review from mattias-p

Assignees

@MichaelTimbert MichaelTimbert

Labels
V-Patch Versioning: The change gives an update of patch in version.
Projects
None yet
Milestone
v2024.2
Development

Successfully merging this pull request may close these issues.

Check MANIFEST when PR is created (zonemaster-ldns)
4 participants
@MichaelTimbert @matsduf @tgreenx @marc-vanderwal