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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bensons
Copy link

@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

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

@@ -108,6 +120,10 @@ If this is set to true (default) then the migration process will transfer merge

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

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

@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
Member

spruce commented Dec 6, 2021

@bensons Any comments regarding the review?

@phil-blain
Copy link

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...

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