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

Change package path from {packagename}-{version} to {packagename}/{version} #300

Merged
merged 7 commits into from
Mar 30, 2020

Conversation

ruflin
Copy link
Member

@ruflin ruflin commented Mar 25, 2020

The current package path is /package/{packagename}-{version}. EPM will need some info about which packages exist. To make it possible to populate some generic info about a package, the path is changed to /package/{packagename}/{version}. No additional info is added yet but it gives you the option to add it.

This PR makes all changes in the code that are needed on the packaging side to make the above happen. Also all test files are updated.

This is a breaking change for EPM.

@ruflin ruflin requested review from jfsiii and neptunian March 25, 2020 12:52
@ruflin ruflin self-assigned this Mar 25, 2020
@neptunian
Copy link
Contributor

@ruflin
Copy link
Member Author

ruflin commented Mar 25, 2020

@neptunian Could you take a lead on that and then we can merge and deploy close together?

os.MkdirAll(dst, 0755)
err := sh.RunV("cp", "-a", src, dst)
err := sh.RunV("rsync", "-a", src, dst)
Copy link
Member Author

Choose a reason for hiding this comment

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

@kuisathaverat I switched to rsync to have better cross platform consistency. But it is not yet on CI. Is there an easy way to get it there?

@neptunian
Copy link
Contributor

@jfsiii I am thinking we need to change the EPM api to mirror this change

@jfsiii
Copy link

jfsiii commented Mar 26, 2020

@neptunian I don't know if we need to (I believe the two are and should be independent) but we can. There is already a ticket for using name@version for EPM. We could implement that or change it to mirror this.

@neptunian
Copy link
Contributor

@ruflin what exactly is {package}/{packageName} endpoint going to return? Will it be similar to search?package=system&all=true?

@ruflin
Copy link
Member Author

ruflin commented Mar 27, 2020

@neptunian At the moment it will return nothing. This change only gives us the opportunity to discuss further on how we will use this endpoint. If it turns out, we don't need it at all this is ok, but I would still like to do the change as if we realise later we need it, we can't make this change anymore as it is a breaking change.

About the content it could return: I'm thinking of a much more compact version than what we have in the /search endpoint which only returns all the available versions potentially with compatiblity.

Related to the comment from @jfsiii: Agree that we don't need to tie this change to the internal EPM API. This can change on its own pace and see there if it makes to change it at all.

Copy link
Contributor

@neptunian neptunian left a comment

Choose a reason for hiding this comment

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

Can you update the readme API?

@ruflin
Copy link
Member Author

ruflin commented Mar 27, 2020

@neptunian README updated.

README.md Outdated
@@ -20,8 +20,7 @@ The `/search` API endpoint has few additional query parameters. More might be ad
a package requires 7.4, the package will not be returned or an older compatible package will be shown.
By default this endpoint always returns only the newest compatible package.
* category: Filters the package by the given category. Available categories can be seend when going to `/categories` endpoint.
* package: Filters by a specific package name, for example `mysql`. In contrast to the other endpoints, it will return
by default all versions of this package.
* package: Filters by a specific package name, for example `mysql`. It will return the most recent version of the package.
Copy link
Contributor

@neptunian neptunian Mar 27, 2020

Choose a reason for hiding this comment

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

@ruflin i think you meant to put this in the search PR. I was referring to updating the API endpoint path on line 12

Copy link
Member Author

Choose a reason for hiding this comment

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

@neptunian argh, sorry. Now did the right adjustement.

…rsion}

The current package path is `/package/{packagename}-{version}`. EPM will need some info about which packages exist. To make it possible to populate some generic info about a package, the path is changed to `/package/{packagename}/{version}`. No additional info is added yet but it gives you the option to add it.

This PR makes all changes in the code that are needed on the packaging side to make the above happen. Also all test files are updated.

This is a breaking change for EPM.
@ruflin ruflin merged commit 667b26e into elastic:master Mar 30, 2020
@ruflin ruflin deleted the new-package-dir branch March 30, 2020 18:59
Copy link

@jfsiii jfsiii left a comment

Choose a reason for hiding this comment

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

When I run this PR locally I see what seems to be old paths values in readme, screenshots.src and icons.src

Screen Shot 2020-03-30 at 2 53 13 PM

Screen Shot 2020-03-30 at 2 52 28 PM

"name": "example",
"title": "Example",
"version": "0.0.2",
"readme": "/package/example-0.0.2/docs/README.md",
Copy link

Choose a reason for hiding this comment

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

Suggested change
"readme": "/package/example-0.0.2/docs/README.md",
"readme": "/package/example/0.0.2/docs/README.md",

ruflin added a commit to ruflin/package-registry that referenced this pull request Mar 30, 2020
In elastic#300 the links to the README and icons were not correct.
@ruflin ruflin mentioned this pull request Mar 30, 2020
ruflin added a commit that referenced this pull request Mar 30, 2020
In #300 the links to the README and icons were not correct.
@ruflin
Copy link
Member Author

ruflin commented Mar 31, 2020

Comments from @jfsiii were fixed in #312

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

Successfully merging this pull request may close these issues.

3 participants