Skip to content

Add image regeneration module#496

Merged
felixarntz merged 5 commits intofeature/regenerate-existing-imagesfrom
feature/setup-regenerate-existing-images-module
Aug 25, 2022
Merged

Add image regeneration module#496
felixarntz merged 5 commits intofeature/regenerate-existing-imagesfrom
feature/setup-regenerate-existing-images-module

Conversation

@ankitrox
Copy link
Copy Markdown

@ankitrox ankitrox commented Aug 22, 2022

Summary

This PR adds the regenerate-existing-images module in the plugin. This shall be the first thing to get merged for image regeneration work.

All of the subsequent work related to image regeneration would reside in this module.

Fixes #489

Relevant technical choices

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@ankitrox ankitrox added [Focus] Images no milestone PRs that do not have a defined milestone for release [Type] Epic A high-level project / epic that will encompass several sub-issues [Plugin] Regenerate Existing Images Issues for the Regenerate Existing Images module labels Aug 22, 2022
@ankitrox ankitrox self-assigned this Aug 22, 2022
@ankitrox ankitrox requested a review from jjgrainger August 22, 2022 15:26
Copy link
Copy Markdown
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Left one minor review.

Comment on lines +10 to +12
return function() {
return true;
};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It should be like below?

Suggested change
return function() {
return true;
};
return '__return_true';

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@mukeshpanchal27 True value is temporary and will be replaced by appropriate condition eventually. Also, this snippet will load the module if the returned value is not callable, so I think we will save calling an extra function which eventually returns true which we are already returning.

Please let me know your opinion.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Similar to my comment in #489, I'd argue we don't even need this file right now. We can add it later if we ever want to add custom logic to determine whether the module can load or not.

@mukeshpanchal27 mukeshpanchal27 linked an issue Aug 22, 2022 that may be closed by this pull request
Copy link
Copy Markdown
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@ankitrox Two minor comments below, and one more here: Can you please also add this module to the CODEOWNERS file, similar to the other image modules? For now maybe add @jjgrainger and yourself as owners if that works for you?

Comment on lines +10 to +12
return function() {
return true;
};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Similar to my comment in #489, I'd argue we don't even need this file right now. We can add it later if we ever want to add custom logic to determine whether the module can load or not.

Copy link
Copy Markdown
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Nit-pick change

@ankitrox ankitrox requested a review from JustinyAhin as a code owner August 25, 2022 10:45
ankitrox and others added 2 commits August 25, 2022 16:18
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
Copy link
Copy Markdown
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Great, thank you @ankitrox!

@felixarntz felixarntz merged commit 2c3843b into feature/regenerate-existing-images Aug 25, 2022
@felixarntz felixarntz deleted the feature/setup-regenerate-existing-images-module branch August 25, 2022 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no milestone PRs that do not have a defined milestone for release [Plugin] Regenerate Existing Images Issues for the Regenerate Existing Images module [Type] Epic A high-level project / epic that will encompass several sub-issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create Regenerate existing images module

3 participants