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

feat: stable XAR upload and install #129

Merged
merged 8 commits into from
Nov 8, 2020

Conversation

line-o
Copy link
Member

@line-o line-o commented Oct 31, 2020

A real breakthrough. Finally XAR installation is stable.

- add options to readAll example
- add explanations to app.*
- consistent error handling examples
- the examples should be more readable in general
BREAKING CHANGE: App installation is not compatible with exist 3.6.1!
This is because the collection `/db/system/repo` does not exist. For installation to work reliably the XAR must be present in that collection (at least for existdb v4+).
@line-o
Copy link
Member Author

line-o commented Nov 1, 2020

As stable as XAR handling currently is, see eXist-db/exist#3598

- extract XQuery scripts into separate files
- add type hints to exported methods
- add and stabilize tests
Copy link
Contributor

@duncdrum duncdrum left a comment

Choose a reason for hiding this comment

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

Looks good, although I m a bit worried about the dependency resolution, I remember some deeply broken instance in the past if a package is present but it’s dependencies can’t be found. Should we maybe perform a check if a dependency is declared, and either issue a warning, or more radically abort install of xars with dependencies?

@line-o
Copy link
Member Author

line-o commented Nov 8, 2020

@duncdrum Thanks for having a look!

After a lot of testing this is the only real path to break it reproducibly: eXist-db/exist#3598

About dependency resolution and broken expath-repositories:

  1. It is mentioned in the readme that dependencies will not be resolved
  2. I have not seen that being an issue (in stark contrast to what happens if you do not remove the old version first, which is now handled by the query).
  3. automatic dependency resolution needs a public-repo url and uses other functions. This will definitely be added as separate function calls or be added here
  4. The install will fail if dependencies cannot be met (see the package manager code, it is using the same functions).

@line-o
Copy link
Member Author

line-o commented Nov 8, 2020

@duncdrum I stand corrected!
I started doubting my above assumptions and yes you can install a package with a missing dependency that way.
I tested installing exist-jwt, which declares that the crypto-lib in version 1.0.0 or higher needs to be installed.
Well, at least it can be safely updated and also removed. That is a clear indicator the expath-repo is still in OK shape after multiple of such operations.
And I also saw there is no mention of that in the readme. Will definitely add that.

@duncdrum
Copy link
Contributor

duncdrum commented Nov 8, 2020

@line-o did you check if you can restart the intended with the incomplete app installed? If that works we re in the clear, I seem to remember that was a big problem way back when.

@line-o
Copy link
Member Author

line-o commented Nov 8, 2020

I decided to move forward and have dependency resolution in place.
Thanks for nudging :)

Call `repo:install-and-deploy-from-db` with package repository URL.

- add defaultPackageRepo to app component.
- add optional parameter `customPackageRepoUrl` to install method

Add optional object "result" to typedef for NormalizedQueryResult.

- adapt xquery return values
- adapt tests
@line-o line-o merged commit 77548c1 into eXist-db:master Nov 8, 2020
@line-o
Copy link
Member Author

line-o commented Nov 8, 2020

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@line-o line-o added the released label Nov 8, 2020
@line-o line-o deleted the fix/stable-app-install branch September 20, 2021 23:11
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.

2 participants