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: Sandstorm: Upgrade Sandstorm package with permissions #2102

Merged
merged 1 commit into from Oct 20, 2023

Conversation

ocdtrekkie
Copy link
Contributor

I doubt your CI can test Sandstorm, so I'm going to go ahead and skip CI here. :)

This PR updates the base box and vagrant-spk configuration to a more modern standard and fixes a couple issues with the build. HLedger already appears to correctly configure permissions thanks to @zarybnicky. I need to go grab my signing key to get a new package built, but I will endeavor to get a grain setup with some test URLs with different permission levels for testing shortly. (You may wish to wait until I get this part complete before merging, in case we find a bug.)

Note that I used Hacktoberfest as my incentive to handle some long-overdue PRs, so while it's not required, if you want to give me credit, please add a hacktoberfest-accepted label to this PR.

@simonmichael
Copy link
Owner

For the record: this comes from the recent #425 (comment) comment on the old #425 issue, and updates the hledger app on sandstorm to (a) modern sandstorm and (b) modern hledger (1.9.2 -> 1.31). You could mention the hledger bump in the commit message ?

Great! I'm ready to merge when you are.

@simonmichael
Copy link
Owner

PS begin the commit message with semicolon to bypass lengthy CI building.

@simonmichael simonmichael added packaging Dependencies, version constraints, packaging.. platform:sandstorm The Sandstorm.io web app hosting platform. labels Oct 20, 2023
@simonmichael
Copy link
Owner

And maybe briefly mention the new permissions abilities - any Sandstorm hledger user docs that need updating ?

@ocdtrekkie
Copy link
Contributor Author

Ah I didn't realize it was the commit message not the PR message that needed the semicolon.

I will check our metadata/documentation stuff prior to merge here.

@ocdtrekkie
Copy link
Contributor Author

I tried amending the commit description and it didn't help skip CI: fatal: ambiguous argument 'origin/update-sandstorm': unknown revision or path not in the working tree. I am wondering if it's because I force-pushed the branch.

Anyways, I also removed some legacy cruft from the description and updated the changelog.

@ocdtrekkie
Copy link
Contributor Author

I would probably use private browsing tabs as a guest to avoid mixing/accumulating permissions, but these three URLs should let us test the same test ledger with different levels of access:

Full access URL: https://ocdhost.sandcats.io/shared/inXFsQuY2d3Fz5556TaPYqmUHxOX_KKAx5fe4H5OvSP
Write access URL: https://ocdhost.sandcats.io/shared/OMLzPhhWxgTY3Z2YSIksCClAPhTyjKwubkzkg5RYMig
View only URL: https://ocdhost.sandcats.io/shared/3wrUmnXsM93FsQvN7JcK_WiEQpSen5buPJyWW6xiYZG

@simonmichael
Copy link
Owner

Yes, probably the force push confused it. Unfortunately I don't know how to reliably detect the recent commits in all situations on github.

@simonmichael
Copy link
Owner

Thank you very much!

@simonmichael simonmichael merged commit b5bf0cb into simonmichael:master Oct 20, 2023
1 check passed
@ocdtrekkie ocdtrekkie deleted the update-sandstorm branch October 20, 2023 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packaging Dependencies, version constraints, packaging.. platform:sandstorm The Sandstorm.io web app hosting platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants