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.

Sign up for GitHub

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

Jump to bottom

feat: fixes and tests for autoreplace support for replacements #17467

Closed
wants to merge 1 commit into from
Closed

feat: fixes and tests for autoreplace support for replacements #17467

wants to merge 1 commit into from

Conversation

t-kulmburg
Copy link
Contributor

As we are also interested in the replace feature, I had a look at the proposed code changes.
To make my changes more clear, I am creating this PR against @JamieMagee's PR
I hope this is also alright with you @JamieMagee.
After fixing building errors I added the name-replacement into the doAutoReplace function as they share most of their logic.
If you have any good ideas for better structure, I can refactor the code further.

I also crated a gitlab demo-repo containing a dependency for all managers listed in the PR of @JamieMagee.
As can be seen in the repo README and the Renovate created merge requests, more then half of them are already working with my rather minimal changes.
I also added a brief description of the problems the other managers currently have.
To test the feature even further I added unit tests for Managers that worked in the demo-repo.

My question now would be how you would like to proceed with this topic in general.
Getting the feature to work for all supported managers will probably require a lot more work,
but supporting only the currently working ones will require a detailed documentation on the current status.

I am willing to spend some more time on this in the next days/weeks, so reviews would be appreciated.

@t-kulmburg
Copy link
Contributor Author

I tested this after merging with develop. Currently without merging the tests are working when started separately, but all at once run out of memory.

@t-kulmburg t-kulmburg marked this pull request as draft August 29, 2022 11:27
viceice
viceice requested changes Aug 29, 2022
View reviewed changes
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

Please start a fresh PR and target main instead.

Don't do refactorings in this feature PR. extract required refactorings to a seperate PR, which should merged before this feature.

lib/workers/repository/update/branch/auto-replace.spec.ts
Comment on lines -212 to -226
it('handles already replaced', async () => {
const dockerfile = 'FROM library/ubuntu:20.04';
upgrade.manager = 'dockerfile';
upgrade.updateType = 'replacement';
upgrade.depName = 'library/alpine';
upgrade.newName = 'library/ubuntu';
upgrade.packageFile = 'Dockerfile';
const res = await doReplacementAutoReplace(
upgrade,
dockerfile,
reuseExistingBranch
);
expect(res).toBe(dockerfile);
});

Copy link
Member

Choose a reason for hiding this comment

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

why this case is removed?

lib/workers/repository/update/branch/auto-replace.spec.ts
expect(res).toBe(hcl.replace(upgrade.depName, upgrade.newName));
});

it('fails with old name in depname', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

does this no longer fail?

lib/workers/repository/update/branch/auto-replace.spec.ts
Comment on lines +242 to +244
upgrade = {
...JSON.parse(JSON.stringify(defaultConfig)),
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
upgrade = {
...JSON.parse(JSON.stringify(defaultConfig)),
};
upgrade = getConfig();

getConfig will always return a fresh config

@viceice
Copy link
Member

viceice commented Aug 29, 2022

I tested this after merging with develop. Currently without merging the tests are working when started separately, but all at once run out of memory.

you should start fresh from main, then the memory issue should be gone

@t-kulmburg
Copy link
Contributor Author

Ok so just that I understand you correctly, you just want me to create a new PR with this changes to main?
(and after considering your comments of course)

Just because I started from @JamieMagee 's PR and refactored/fixed his work, and I did not want to just create one and make it seem like I did this all on my own.

@viceice
Copy link
Member

viceice commented Aug 29, 2022

Ok so just that I understand you correctly, you just want me to create a new PR with this changes to main? (and after considering your comments of course)

Just because I started from @JamieMagee 's PR and refactored/fixed his work, and I did not want to just create one and make it seem like I did this all on my own.

no problem @JamieMagee is very busy, so i 'm pretty sure he's happy when somebody finish his work.

@JamieMagee
Copy link
Contributor

@t-kulmburg thanks for opening this. It looks like you opened #17476 targeting main, instead of my branch. Does that mean we can close this one?

@viceice viceice closed this Aug 29, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@viceice viceice viceice requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@t-kulmburg @viceice @JamieMagee