Skip to content

new Settings for migration when assignee users don't exist and S3 isn't used for attachment storage#102

Closed
bensons wants to merge 1 commit intopiceaTech:masterfrom
bensons:master
Closed

new Settings for migration when assignee users don't exist and S3 isn't used for attachment storage#102
bensons wants to merge 1 commit intopiceaTech:masterfrom
bensons:master

Conversation

@bensons
Copy link
Copy Markdown

@bensons bensons commented Sep 30, 2021

Created settings for clearing issue assignees, bypassing S3 bucket upload, saving issue
attachments locally, and rewriting attachment URLs in issue content. Fixed problem downloading
attachments with international characters by encoding attachment download URIs.

…load, saving issue

attachments locally, and rewriting attachment URLs in issue content. Fixed problem downloading
attachments with international characters by encoding attachment download URIs.
@ujos
Copy link
Copy Markdown

ujos commented Oct 19, 2021

Very cool commit. I have a few notes from my side if I could:

  1. Would be great if you could split clearIssueAssignment, attachment URI encoding and S3 stuff so they can be applied separately.
  2. useS3 flag conflicts with overrideURL flag. I would use overrideURL only if useS3 is FALSE


As default it is set to false. Doesn't fire the requests to github api and only does the work on the gitlab side to test for wonky cases before using up api-calls

#### clearIssueAssignment
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I believe would be better to have two commits:

  1. One commit for keepLocal+overrideURL+useSe
  2. Another commit for clearIssueAssignment

) {
// Get GitHub username from settings
props.assignees.push(settings.usermap[pullRequest.assignee.username]);
if (pullRequest.assignee){
Copy link
Copy Markdown

@ujos ujos Oct 19, 2021

Choose a reason for hiding this comment

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

The space character removed between ) {... For cleaner commit would be better to keep original formatting

@spruce
Copy link
Copy Markdown
Member

spruce commented Dec 6, 2021

@bensons Any comments regarding the review?

@phil-blain
Copy link
Copy Markdown

I've used the code in this PR to transfer the images to a hidden ref in the repo and use a relative link to have them show up in the issues, i.e. something like ../blob/<hash>/path/to/image.png?raw=true (cf. github/docs#17479).

Since the PR does not merge cleanly anymore, here is an up-to-date version if it helps anyone (I don't have time right now to clean it up and submit a proper PR): master...phil-blain:fixes. The only thing I did not reapply is the clearIssueAssignment setting.

I've also removed the use of the githubRepoId in the relative path as this solves a chicken-and-egg problem where if you don't know the repo ID before running the conversion, you can't use it in the overrideURL setting...

@bensons bensons closed this Apr 29, 2025
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.

4 participants