-
Notifications
You must be signed in to change notification settings - Fork 2
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
Prep before publishing action #3
Conversation
atoomic
commented
Apr 27, 2024
•
edited
Loading
edited
- Update Markdown to advertise perl-actions/perl-versions
- Bump node to v20
- Make since-perl optional
- Add .github/check.yml
- use ncc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, except of comments it looks reasonable
matrix: | ||
perl-version: ${{ fromJson (needs.since-536-with-devel.outputs.perl-versions) }} | ||
container: | ||
image: perldocker/perl-tester:${{ matrix.perl-version }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi, perl-versions are used to publish perl-tester docker images, so upcoming v5.40 will be first added here and perl-tester:5.40 will exist later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeap I was expecting such addition in a few days, but looks like we control the list manually and at the same time we are adding it we should now also add it to the poor man test I ve added there, should be ok I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, let's see, we can fix issues later
workflow I have growing in my mind is:
- test version validity using authority list (probably reusing
perlbrew available
) - trigger perl-tester publish workflow when new version is added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitively good improvements we can add in a short future!
I'm trying to get that pipeline ready for release so we can start using it in multiple distro
Let me know if you are fine with the current state before merging
thanks
Rather than including node_modules in the repository, build a single file containing everything needed using @vercel/ncc. The file can be rebuilt using npm run build.
@happy-barney this is now ready for a second review, note that I've added some extra improvements such as Would definitively like to move forward with that change so I could then release the action and start consuming it in different distro. thanks |
//const github = require('@actions/github'); | ||
const semver = require('semver'); | ||
|
||
try { | ||
const since_perl = semver.coerce (core.getInput('since-perl')); | ||
const with_devel = core.getInput('with-devel') == "true"; | ||
const since_perl = semver.coerce(core.getInput('since-perl')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please retain indent with tabs. That is something I call "contributor friendly", allowing everyone to set their own tab width to whatever they want, letting tab representing "1 indent level"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added pretty
and this is auto converted by npm run pretty
core.setFailed(error.message); | ||
} | ||
core.setFailed(error.message); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please also configure tooling to enforce trailing newline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same this is coming from pretty
I'm sure this is configurable, not sure how, feel free to adjust
good improvements, thanks |