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

Commented updated hook. #383

Merged

Conversation

MdNadimHossain
Copy link
Contributor

Jira

https://digital-vic.atlassian.net/browse/SDPAP-7108

Problem/Motivation

  1. It causes issues for tide_site as tide_site not dependant on tide_landing_page and creates a garbage field as the field is not provided by tide_core. This hook should have been added in tide_landing_page
  2. It causes issues for tide_landing_page as well, cause tide_landing_page already has this field in the active config and then tide_core tries to add it again while running this hook and creates conflicts

Fix

  1. Commenting out the hook so that it does not run this on other modules where tide_core is added as a dependency.

Related PRs

Screenshots

TODO

@MdNadimHossain
Copy link
Contributor Author

I just learned that update hook runs on a fresh install for the modules that are added as a dependency. Also this issue is an example of why a related update hook should be added to the module by which the related fields are provided.

Copy link
Contributor

@anthony-malkoun anthony-malkoun left a comment

Choose a reason for hiding this comment

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

Just remove it. Have a comment referring to the ticket if you want but remove the code.

@MdNadimHossain
Copy link
Contributor Author

I just learned that update hook runs on a fresh install for the modules that are added as a dependency. Also this issue is an example of why a related update hook should be added to the module by which the related fields are provided.

I take this statement back, it does not runs the update hook during fresh install. tide_core has a logic set in here which runs all the update hooks except for the ones defined in the array. But I still believe this particular update hook should not be here as this module is not the field provider, so removing the code from here.

@vincent-gao
Copy link
Contributor

LGTM @MdNadimHossain

@MdNadimHossain MdNadimHossain merged commit 1524be3 into develop Feb 28, 2023
@MdNadimHossain MdNadimHossain deleted the bug/SDPAP-7108-build-fail-issue-on-depenant-modules branch February 28, 2023 01:56
@anthony-malkoun
Copy link
Contributor

Maybe too late now, but I think the function signature needs to be there. Might break or not run later hooks on a clean install.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants