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

21 remove vocabs from deleted branches #22

Merged
merged 11 commits into from
Feb 13, 2024

Conversation

sroertgen
Copy link
Contributor

@sroertgen sroertgen commented Feb 1, 2024

This PR adds the feature to remove unused vocabularies (#21).

You can make a dry run with npm run cleanup-dist:dry
To actually remove the unused vocabularies run npm run cleanup-dist

The output of this script as well as the rebuild-vocabs logs are now written to a log file log.log. (#5)

There is now also an endpoint to get all currently served vocabularies: /currentVocabs.
It will output information like this:

[
	{
		"repository": "sroertgen/test-vocabs",
		"vocabulary": "http://localhost:8000/sroertgen/test-vocabs/heads/main/",
		"date": "2024-02-01T08:59:36.730Z"
	}
]

Besides I was able to prettify the build logs and remove those strange character:

Before:
image

After:
image

Closes #21
Closes #5

@sroertgen
Copy link
Contributor Author

sroertgen commented Feb 1, 2024

@Phu2 Please deploy to test for review.

You will have to set a new env variable (sth like VOCABS_URL=test.skohub.io), see also README

Feel free to message me if we want to try out the cleanup.

@Phu2
Copy link
Contributor

Phu2 commented Feb 1, 2024

Deployed to test.

Cleanup works great with docker run -v ./log.log:/app/log.log -v ./.env:/app/.env -v ./dist:/app/dist skohub-webhook:latest "npm run cleanup-dist:dry" (mapped log.log to host).

Added RewriteRule for the new endpoint:
RewriteRule ^/currentVocabs$ http://10.9.0.41:3000/currentVocabs [P,L]

@sroertgen
Copy link
Contributor Author

Note: I renamed the endpoint to /vocabs since the "current" is kind of superfluous

@sroertgen
Copy link
Contributor Author

Assigning @acka47 now for functional review.

The cleanup part was already successfully tested with Phu.

What you can test:

@sroertgen sroertgen requested a review from acka47 February 5, 2024 08:08
@sroertgen sroertgen assigned acka47 and unassigned Phu2 Feb 5, 2024
acka47
acka47 previously approved these changes Feb 8, 2024
Copy link
Member

@acka47 acka47 left a comment

Choose a reason for hiding this comment

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

+1

@acka47 acka47 self-requested a review February 8, 2024 09:26
@acka47 acka47 dismissed their stale review February 8, 2024 09:26

found some things

@acka47
Copy link
Member

acka47 commented Feb 8, 2024

After approving I realized:

  1. I'd like to have some additional information in the build (link to the build log page).
  2. We should have the complete URL to the repo as we want to support other forges in the future (related, but for the docker version: Check alternatives to GitHub actions and pages skohub-pages#19 )
  3. We should generally use functioning URLs that include the protocol (http/https)

As a result, I imagine it to look something like this:

{
   "repository":"https://github.com/acka47/testing-skohub-vocabs",
   "vocabulary":"https://test.skohub.io/acka47/testing-skohub-vocabs/heads/master/",
   "latestBuild":{
      "timestamp":"2024-02-07T12:06:20.475Z",
      "id":"https://test.skohub.io/build?id=93cc2ad1-2d0a-4f19-971e-b3a4d764614b"
   }
}

@sroertgen
Copy link
Contributor Author

@Phu2 Please deploy on test system. Be aware of the above mentioned change in naming the endpoint.

@Phu2
Copy link
Contributor

Phu2 commented Feb 8, 2024

Deployed to test and changed RewriteRule for renamed endpoint https://test.skohub.io/vocabs

@Phu2 Phu2 removed their assignment Feb 8, 2024
@acka47
Copy link
Member

acka47 commented Feb 8, 2024

This looks much better but one point isn't adressed yet:

We should generally use functioning URLs that include the protocol (http/https)

I'd like to see https://test.skohub.io/hbz/lobid-vocabs/heads/209-addAddnotesULBMuenster/ instead of test.skohub.io/hbz/lobid-vocabs/heads/209-addAddnotesULBMuenster/ and https://test.skohub.io/build?id=342df7c8-5c3e-451e-b3b0-e64a3ce6f1c3 instead of test.skohub.io/build?id=342df7c8-5c3e-451e-b3b0-e64a3ce6f1c3

@sroertgen
Copy link
Contributor Author

This looks much better but one point isn't adressed yet:

We should generally use functioning URLs that include the protocol (http/https)

I'd like to see https://test.skohub.io/hbz/lobid-vocabs/heads/209-addAddnotesULBMuenster/ instead of test.skohub.io/hbz/lobid-vocabs/heads/209-addAddnotesULBMuenster/ and https://test.skohub.io/build?id=342df7c8-5c3e-451e-b3b0-e64a3ce6f1c3 instead of test.skohub.io/build?id=342df7c8-5c3e-451e-b3b0-e64a3ce6f1c3

That is related to deployment.
@Phu2 Please change the .env variables accordingly and add the protocol.

@Phu2
Copy link
Contributor

Phu2 commented Feb 8, 2024

Changed and redeployed to test.

@Phu2 Phu2 removed their assignment Feb 8, 2024
Copy link
Member

@acka47 acka47 left a comment

Choose a reason for hiding this comment

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

Looks great now. I love it. +1

@acka47 acka47 removed their assignment Feb 13, 2024
@sroertgen sroertgen merged commit ac148f2 into main Feb 13, 2024
@sroertgen sroertgen deleted the 21-remove-vocabs-from-deleted-branches branch February 13, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
3 participants