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: support url rewrite #1210

Merged
merged 12 commits into from
Jul 22, 2023
Merged

feat: support url rewrite #1210

merged 12 commits into from
Jul 22, 2023

Conversation

viceice
Copy link
Member

@viceice viceice commented Jul 18, 2023

We now call back into our new cli to download the file, so the new url replacement can be applied.
Will add some more documentation too later.

@viceice viceice requested review from Chumper and rarkins July 18, 2023 08:17
@Chumper
Copy link
Collaborator

Chumper commented Jul 18, 2023

We can phase out the bats tests whenever we replace functionality with typescript

@viceice
Copy link
Member Author

viceice commented Jul 18, 2023

We can phase out the bats tests whenever we replace functionality with typescript

fixed the tests. I'm not sure if i need the file delete 🤔

@viceice
Copy link
Member Author

viceice commented Jul 18, 2023

We can phase out the bats tests whenever we replace functionality with typescript

fixed the tests. I'm not sure if i need the file delete 🤔

https://stackoverflow.com/questions/37176783/node-js-fs-more-efficient-way-to-create-file-clear-file

fs.createWriteStream will truncate exisitng file

@viceice viceice marked this pull request as ready for review July 18, 2023 10:06
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@viceice viceice requested a review from Chumper July 18, 2023 11:47
Copy link
Member

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

Can we get docs for the URL rewriting if this closes the issue?

@viceice
Copy link
Member Author

viceice commented Jul 18, 2023

Can we get docs for the URL rewriting if this closes the issue?

there are already some docs and all urls are printed on debug log level

@viceice
Copy link
Member Author

viceice commented Jul 19, 2023

P My

???

@rarkins
Copy link
Member

rarkins commented Jul 20, 2023

Not sure where that came from. Can you point me to the docs?

@viceice
Copy link
Member Author

viceice commented Jul 20, 2023

Not sure where that came from. Can you point me to the docs?

https://github.com/containerbase/base/tree/feat/url-replace#url-replacement

will create a mother extended version in later PR's.

will try to extract the base urls to variables, so I can use a test to validate docs to have a sample for each of them.

Copy link
Member

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

Don't want to block this but we need conclusive docs for rewriting all URLs in a "full" image

@viceice
Copy link
Member Author

viceice commented Jul 22, 2023

yes, will update docs later. currently it's best to try and error to see failed urls in logs. then it should be easy to rewrite them.

@viceice viceice added this pull request to the merge queue Jul 22, 2023
Merged via the queue into main with commit cdec808 Jul 22, 2023
@viceice viceice deleted the feat/url-replace branch July 22, 2023 10:53
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.

Allow all external download URLs to be aliased
3 participants