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

Add SHA256 checksum to package info #39

Merged
merged 5 commits into from
May 13, 2019
Merged

Add SHA256 checksum to package info #39

merged 5 commits into from
May 13, 2019

Conversation

adamretter
Copy link
Member

No description provided.

@joewiz
Copy link
Member

joewiz commented May 12, 2019

My only worry about deploying a new version of the public-repo app based on this PR to demo server is that, in my testing (with eXist 5.0.0-RC8, mavenized), the "public" directory containing all uploaded xars is not being backed up—and thus would be easy to lose the old directory without a manual backup/copy of this directory.

It used to be that the pre-install.xql would fire before eXist deleted the existing installation, but for some reason, it is not. Has anything changed in the way pre-install.xql works, relative to the deletion of an existing app being replaced with a new app?

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.

changes look good to me. Thanks @adamretter
i'd feel a lot better if there were tests for public repo

@duncdrum
Copy link
Contributor

@joewiz if pre-install.xql is not firing correctly this sounds like a bug in package-services could you maybe add a test there and see what happens?

@adamretter
Copy link
Member Author

@joewiz the demo server is still running an old eXist-db 4.x x

Regards preinstall.xql, if that is not firing on the Mavenized exist-db for some reason we should cope with that as a separate issue. AFIK it should work in the same way as 4.x.x

@joewiz
Copy link
Member

joewiz commented May 12, 2019

@duncdrum By "package-services", do you mean https://github.com/eXist-db/existdb-packageservice? I was bypassing that in my tests, uploading the xar to a temporary directory, calling repo:install-and-deploy-from-db. (I also tested uploading via dashboard, which would've exercised the package manager / service, and the result was the same.) Even though pre-install.xql is supposed to fire before the old app is removed, it appears to be firing after removal, so the installation step was losing the opportunity to backup the public directory. Adding more tests for this app and package management in general would be wonderful.

@adamretter If the version of eXist on the demo server doesn't change (it's 4.1.0), we should be fine to install new versions of the public-repo app, since the behavior in question has worked for me without problem for the past several releases I coordinated. In light of this, though, we'll just need to exercise extra caution and back up the existing xars during such operations until we get a handle on the phenomenon I observed.

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.

Ok now i m both confused and nervous,
@adamretter the min-version of this PR is 3.5.0 so it should work as before on 4.1.0, if it doesn't we need to change the processor dependency

@joewiz loosing backups of ones repo could be very bad, both for us and others. So let's get to the bottom of this. Does this only happen in the mavenized version or also on demo.exist-db.org both when updating via dashboard and for repo:install-and… or only one? In either case we should add a test PR to whereever that error lives and link it here.

@joewiz
Copy link
Member

joewiz commented May 12, 2019

  1. Public repo never had a semver-min, so Adam chose 3.5.0 since this is the semver-min for the v0.5 version of the expath-crypto-exist-lib dependency he set. I only mentioned 4.1.0 since that is the version of eXist running on the demo server. I don't see a problem with this aspect of the PR.
  2. I agree that more testing would be helpful, but it needn't hold up deployment of this PR if it's needed to orchestrate other pressing changes. It appears that Adam's PR is already live on the demo server, and the xars are all intact. If that's the case, let's merge this, publish a release and a tag, and align the server with the repo.
    @duncdrum Could you please resolve your requested changes for this PR so we can merge and proceed?
    And whoever published the PR to the public-repo, could you please advise if any special steps were needed when you published, or if it worked smoothly without any other manual intervention?

@duncdrum
Copy link
Contributor

duncdrum commented May 12, 2019

@joewiz you re clearly describing a bug, if public repo isn t creating a backup in accordance with its pre-install instructions. Please open a corresponding issue with steps to reproduce or better yet a test showing the behavior.

I m not aware of any hot-fix worthy problems that warrant a release of a broken update within 24h of creating this PR, I m in my third week of a waiting review for the docs that’s just how it goes sometimes. I thought the whole point of this review dance is so we don t release broken stuff and ask questions later.

@joewiz
Copy link
Member

joewiz commented May 13, 2019

@duncdrum We have no evidence that the issue I described is related to this PR. I will do further testing and will file a bug report if I find there is a real problem. Also, while code from this PR was apparently (?) released to the demo server before the PR was merged, the PR itself does not contain release preparations, so I think we should set that aside as from the review itself.

Are there any changes to this PR that you're requesting?

@joewiz joewiz merged commit 000a461 into master May 13, 2019
@adamretter
Copy link
Member Author

@joewiz thanks :-)

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

Successfully merging this pull request may close these issues.

None yet

3 participants