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

Prevent unexisting aliases #276

Merged
merged 4 commits into from
Oct 11, 2021

Conversation

mfolkeseth
Copy link
Contributor

@trygve-lie
Copy link
Contributor

Great. Thanks for this :)

There is a fair bit of lint adjustments here, can you give a short description of what the changes are? Also for the sake of the release notes.

@trygve-lie
Copy link
Contributor

It was basically missing a validation of the version if I read this correctly. Right?

@mfolkeseth
Copy link
Contributor Author

mfolkeseth commented Oct 8, 2021

That is correct. It does a check for package version before the alias is updated 👍

Also, I gave you an @ for the entry point for the fix.

@trygve-lie
Copy link
Contributor

I guess this adds a 400 - BadRequest to the alias POST documentation: https://eik.dev/docs/server_rest_api#update-alias

@trygve-lie
Copy link
Contributor

Are you able to update the tests?

@mfolkeseth
Copy link
Contributor Author

It leaves a throw new HttpError.NotFound(); - 404 error to follow the flow of the cli https://github.com/eik-lib/cli/blob/master/classes/alias.js#L101. In which case no doc update is needed. IMO the API and CLI feedback is just fine, just missing this additional package version check.

Yes, I will absolutely add some tests. I also think it would be beneficial to pull back the lint fix in this PR to keep it more clean.

@trygve-lie
Copy link
Contributor

It leaves a throw new HttpError.NotFound(); - 404 error to follow the flow of the cli https://github.com/eik-lib/cli/blob/master/classes/alias.js#L101. In which case no doc update is needed. IMO the API and CLI feedback is just fine, just missing this additional package version check.

The API and CLI seems to be fine but there is a new status code in the POST method not reflected in the doc but I can check and update the doc when this is merged :)

Yes, I will absolutely add some tests.

Seems like there is just two tests failing due to the change in the returned http status code so it should be to just alter the status code in the tests.

If your able to add a test for the version check its great 👍

I also think it would be beneficial to pull back the lint fix in this PR to keep it more clean.

That would be great but might break the build. If it does its no stress. I see whats done here.

@mfolkeseth
Copy link
Contributor Author

One small alteration to the implementation; I thought it would make more sense to just check if <version>.package.json exists. The implementation does not check the contents if this file anyway.

Also, I added the <version>.package.json to test sink for existing tests and did a separate test for cases where the package origin file does not exist.

As for the release notes:

  • Make sure package version exists before creating/updating alias.

@trygve-lie
Copy link
Contributor

Looks good to me 👍

@trygve-lie trygve-lie merged commit a5b9601 into eik-lib:master Oct 11, 2021
github-actions bot pushed a commit that referenced this pull request Oct 11, 2021
## [1.2.24](v1.2.23...v1.2.24) (2021-10-11)

### Bug Fixes

* prevent unexisting aliases ([#276](#276)) ([a5b9601](a5b9601))
@github-actions
Copy link

🎉 This PR is included in version 1.2.24 🎉

The release is available on:

Your semantic-release bot 📦🚀

@trygve-lie
Copy link
Contributor

Released :) We depend on semantic release and renovate bot for auto release and dependency updates in this project so it should propagate to the service module by it self soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

npx eik package-alias allows setting alias to non existant version
2 participants