PaloAltoNetworks / pan-cnc

CNC: Chevy's, not Cadillacs. Rapid UI prototyping for all Palo Alto Networks WWSE demos and pocs.
Apache License 2.0
3 stars 3 forks source link

submodule update fix #34

Closed andrewmallory closed 3 years ago

andrewmallory commented 3 years ago

For some reason git python doesn't actually clone down the files when you switch branches from a branch without a submodule to one with a submodule. The submodule would be aware of the files it should have, but they wouldn't be on disk, which caused errors in panhandler.

Not 100% sure yet why but this fixed it for me, basically a different approach to the same command.

nembery commented 3 years ago

This is a good catch. However, while we are here, we should probably add a couple more options as well to be safe.

From the doc string:

        """Update the submodules of this repository to the current HEAD commit.
        This method behaves smartly by determining changes of the path of a submodules
        repository, next to changes to the to-be-checked-out commit or the branch to be
        checked out. This works if the submodules ID does not change.
        Additionally it will detect addition and removal of submodules, which will be handled
        gracefully.

        :param previous_commit: If set to a commit'ish, the commit we should use
            as the previous commit the HEAD pointed to before it was set to the commit it points to now.
            If None, it defaults to HEAD@{1} otherwise
        :param recursive: if True, the children of submodules will be updated as well
            using the same technique
        :param force_remove: If submodules have been deleted, they will be forcibly removed.
            Otherwise the update may fail if a submodule's repository cannot be deleted as
            changes have been made to it (see Submodule.update() for more information)
        :param init: If we encounter a new module which would need to be initialized, then do it.
        :param to_latest_revision: If True, instead of checking out the revision pointed to
            by this submodule's sha, the checked out tracking branch will be merged with the
            latest remote branch fetched from the repository's origin.
            Unless force_reset is specified, a local tracking branch will never be reset into its past, therefore
            the remote branch must be in the future for this to have an effect.
        :param force_reset: if True, submodules may checkout or reset their branch even if the repository has
            pending changes that would be overwritten, or if the local tracking branch is in the future of the
            remote tracking branch and would be reset into its past.
        :param progress: RootUpdateProgress instance or None if no progress should be sent
        :param dry_run: if True, operations will not actually be performed. Progress messages
            will change accordingly to indicate the WOULD DO state of the operation.
        :param keep_going: if True, we will ignore but log all errors, and keep going recursively.
            Unless dry_run is set as well, keep_going could cause subsequent/inherited errors you wouldn't see
            otherwise.
            In conjunction with dry_run, it can be useful to anticipate all errors when updating submodules
        :return: self"""        """Update the submodules of this repository to the current HEAD commit.
        This method behaves smartly by determining changes of the path of a submodules
        repository, next to changes to the to-be-checked-out commit or the branch to be
        checked out. This works if the submodules ID does not change.
        Additionally it will detect addition and removal of submodules, which will be handled
        gracefully.

        :param previous_commit: If set to a commit'ish, the commit we should use
            as the previous commit the HEAD pointed to before it was set to the commit it points to now.
            If None, it defaults to HEAD@{1} otherwise
        :param recursive: if True, the children of submodules will be updated as well
            using the same technique
        :param force_remove: If submodules have been deleted, they will be forcibly removed.
            Otherwise the update may fail if a submodule's repository cannot be deleted as
            changes have been made to it (see Submodule.update() for more information)
        :param init: If we encounter a new module which would need to be initialized, then do it.
        :param to_latest_revision: If True, instead of checking out the revision pointed to
            by this submodule's sha, the checked out tracking branch will be merged with the
            latest remote branch fetched from the repository's origin.
            Unless force_reset is specified, a local tracking branch will never be reset into its past, therefore
            the remote branch must be in the future for this to have an effect.
        :param force_reset: if True, submodules may checkout or reset their branch even if the repository has
            pending changes that would be overwritten, or if the local tracking branch is in the future of the
            remote tracking branch and would be reset into its past.
        :param progress: RootUpdateProgress instance or None if no progress should be sent
        :param dry_run: if True, operations will not actually be performed. Progress messages
            will change accordingly to indicate the WOULD DO state of the operation.
        :param keep_going: if True, we will ignore but log all errors, and keep going recursively.
            Unless dry_run is set as well, keep_going could cause subsequent/inherited errors you wouldn't see
            otherwise.
            In conjunction with dry_run, it can be useful to anticipate all errors when updating submodules
        :return: self"""

I'm thinking the code should be something like:


repo.submodule_update(recursive=True, init=True, force_reset=True, force_remove=True)

The thinking here is panhandler should not be making changes to submodules anyway. Those changes should only happen upstream. Also, added the force_reset and force_remove options in case things get removed / changed / moved / etc upstream.

Thoughts @andrewmallory ?

andrewmallory commented 3 years ago

So after a little playing around with it, seems the best approach is actually probably to use both command. If you don't include the repo.git.submodule version, the files never appear on disk, for some reason repo.submodule_update just doesn't do it.

That said reading through the options I found a shortcoming with the repo.git.submodule command. Switching from develop back to master it doesn't actually remove the submodule from disk, resulting in a branch potentially loading skillets that shouldn't be there. Keeping both commands like this seemds to allow you to switch back and forth freely with expected results. Check the commit I just pushed.

nembery commented 3 years ago

PR accepted and merged. New build is incoming with these fixes