KEMBAR78
Check that tag is valid semver in install.sh by zd4y · Pull Request #556 · dotenv-linter/dotenv-linter · GitHub
Skip to content

Check that tag is valid semver in install.sh #556

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
Merged
merged 1 commit into from
Nov 1, 2022
Merged

Check that tag is valid semver in install.sh #556

merged 1 commit into from
Nov 1, 2022

Conversation

zd4y
Copy link
Contributor

@zd4y zd4y commented Oct 2, 2022

Resolves #539
Add check in install.sh to validate that the tag argument matches semver

✔ Checklist:

@github-actions
Copy link

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the stale label Oct 16, 2022
@mgrachev mgrachev requested a review from a team October 17, 2022 09:58
@mgrachev
Copy link
Member

@zd4y Hi 👋
Thank you for your contribution!

Hmm, linux_install check failed 🤔 Could it be because of your improvements?

@mgrachev mgrachev removed the stale label Oct 17, 2022
@zd4y
Copy link
Contributor Author

zd4y commented Oct 18, 2022

Yes, it is giving the following warnings:

  • SC3010: In POSIX sh, [[ ]] is undefined.
  • SC3057: In POSIX sh, string indexing is undefined.
  • SC3015: In POSIX sh, =~ regex matching is undefined.

I can fix it following the recommendations on the shellcheck wiki of each warning or changing the first line from #!/bin/sh to #!/bin/bash. Is it important to keep the script in sh?

@mgrachev
Copy link
Member

I think yes, let's keep the script in sh.

@dotenv-linter/core Do you agree?

@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2022 •
edited
Loading

Codecov Report

Merging #556 (82c47e0) into master (a526a06) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 82c47e0 differs from pull request most recent head da57771. Consider uploading reports for the commit da57771 to get more accurate results

@@           Coverage Diff           @@
##           master     #556   +/-   ##
=======================================
  Coverage   98.91%   98.91%           
=======================================
  Files          43       43           
  Lines        2761     2769    +8     
=======================================
+ Hits         2731     2739    +8     
  Misses         30       30
Impacted Files Coverage Δ
src/checks/quote_character.rs 100.00% <100.00%> (ø)
src/common/comment.rs 100.00% <100.00%> (ø)
src/common/line_entry.rs 100.00% <100.00%> (ø)
src/common/warning.rs 100.00% <100.00%> (ø)
src/lib.rs 98.50% <100.00%> (-0.03%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mgrachev
Copy link
Member

Please add checks to make sure that everything works.
For example: https://github.com/dotenv-linter/dotenv-linter/blob/master/.github/workflows/ci.yml#L61:L70

zd4y
zd4y commented Oct 23, 2022
View reviewed changes
@mgrachev
Copy link
Member

@zd4y Please fix the following error:

[Invalid workflow file: .github/workflows/ci.yml#L85](https://github.com/dotenv-linter/dotenv-linter/actions/runs/3313181346/workflow)
The workflow is not valid. .github/workflows/ci.yml (Line: 85, Col: 16): Unrecognized named-value: 'matrix'. Located at position 1 within expression: matrix.shell .github/workflows/ci.yml (Line: 90, Col: 16): Unrecognized named-value: 'matrix'. Located at position 1 within expression: matrix.shell

@zd4y
Copy link
Contributor Author

zd4y commented Oct 25, 2022 •
edited
Loading

Hopefully this fixes it, how can I run it in my fork?

@zd4y
Copy link
Contributor Author

zd4y commented Oct 25, 2022

With 2a7e47c I try to fix the jobs that check wrong inputs (I can't test it in my fork or in my pc so I don't know if it works) and I add checks for windows.

The script not working on macos maybe has to do with grep -P not being available in macos? I don't have a mac so I can't test that either. I will probably try to use a different command to fix that later.

@mgrachev
Copy link
Member

The script not working on macos maybe has to do with grep -P not being available in macos? I don't have a mac so I can't test that either. I will probably try to use a different command to fix that later.

Yes, you're right. grep on macOS doesn't have such option:

$ grep -P
grep: invalid option -- P
usage: grep [-abcdDEFGHhIiJLlMmnOopqRSsUVvwXxZz] [-A num] [-B num] [-C[num]]
	[-e pattern] [-f file] [--binary-files=value] [--color=when]
	[--context[=num]] [--directories=action] [--label] [--line-buffered]
	[--null] [pattern] [file ...]

@mgrachev
Copy link
Member

@zd4y Is this PR ready for review?

Also, please squash your commits.

@zd4y
fix: check that tag is valid semver
da57771 
fix: check that tag is valid semver

fix: make sure tag starts with v and update error message

fix: move tag validation to function

fix: replace bash extensions with normal sh

ci: add checks for install with tag

ci: remove tag 'not a tag' from job

ci: replace == with = in test

ci: fix matrix.shell

ci: remove matrix.shell and os:windows-latest

ci: fix install_specific_versions_wrong_input job and add windows jobs

fix: use `grep -E` instead of `grep -P`

ci: fix wrong input jobs

ci: remove non-existent windows releases and redirect install.sh stderr to stdout in wrong input jobs

ci: fix error message test
@zd4y
Copy link
Contributor Author

zd4y commented Nov 1, 2022

@zd4y Is this PR ready for review?

Also, please squash your commits.

Yes, please review it :)

@mgrachev mgrachev merged commit bfca250 into dotenv-linter:master Nov 1, 2022
@mgrachev
Copy link
Member

mgrachev commented Nov 1, 2022

@zd4y Thank you for your help 🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mgrachev mgrachev mgrachev approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add version (argument) pattern check

3 participants

@zd4y @mgrachev @codecov-commenter