Target branch `.sync.yml` not taken into account on branch update (--force)

[bittner]

When updating an existing branch, msync update --force seems to consider the values in the .sync.yml file of the existing branch (meant to be updated), instead of the .sync.yml file of the target branch. This might be a design fault.

Side note: I did this on a GitLab merge request, but the behavior is likely the same even w/o --pr

Example update flow

1. I run msync update --pr, which creates an update (new branch, optional merge request)
2. I figure out that the change was wrong on a target module, due to a value I forgot to update in .sync.yml; hence, I update .sync.yml (in target module, in target branch, e.g. master).
3. I run msync update --pr --force to update the already existing branch (created earlier)

Now, I would expect the updated branch to consider the values in .sync.yml of the target branch (i.e. master in our example). But the changes are based on the values of .sync.yml from the existing, earlier created branch.

I need to delete the branch created by msync update and run ModuleSync afresh. 😟

Recreate target branch? (instead of "update")

The above behavior is probably not intended. The branch was originally created off the target branch, which then naturally has a .sync.yml file that matches across both branches.

When a branch is updated, especially with --force, then the branch should really, technically, be created off that target branch again and force-pushed over the branch created by ModuleSync earlier. That way we would guarantee the same behavior at all times. Correct?

[neomilium]

I had similar troubles using msync but according to the help, --force is not the option for this expected behavior:

$ bundle exec msync help update
Usage:
  msync update

Options:
[...]
      [--force]                                      # Force push amended commit
[...]

IMHO, in msync update we lack of a --reset-hard option, in order to hard-reset on the specified branch before updating files then push again the result.
That's why I add it in #199 at 5942b00.

More precisely, I dont catch the usecase when you want to amend an existing commit then push if we had an hard-reset feature.
AFAICS, amending a commit is more dangerous for mass manipulation, as you dont ever know is the previously commit is what you expected.

[neomilium]

In order to illustrate your issue (and test the new framework available in #202), I wrote what I understand of your issue as a behavior test:

Feature: Update module with a .sync.yml file

  @wip
  @announce-output
  Scenario: Updating a module with a .sync.yml file, then update it upstream
    Given a basic setup with a puppet module "puppet-test" from "fakenamespace"
    And a file named "config_defaults.yml" with:
      """
      ---
      a-managed-file:
        a-key: a-global-value
      """
    And a file named "moduleroot/a-managed-file.erb" with:
      """
      <%= @configs['a-key'] %>
      """
    And the puppet module "puppet-test" from "fakenamespace" has a file named ".sync.yml" with:
      """
      ---
      a-managed-file:
        a-key: an-old-local-value
      """
    When I run `msync update --branch modulesync --message "Update a-managed-file"`
    Then the exit status should be 0
    And the file named "modules/fakenamespace/puppet-test/a-managed-file" should contain:
      """
      an-old-local-value
      """
    And the puppet module "puppet-test" from "fakenamespace" should have 1 commit made by "Aruba" in branch "modulesync"
    Given the puppet module "puppet-test" from "fakenamespace" has a file named ".sync.yml" with:
      """
      ---
      a-managed-file:
        a-key: a-new-local-value
      """
    When I run `msync update --branch modulesync --message "Update a-managed-file" --force`
    Then the exit status should be 0
    And the file named "modules/fakenamespace/puppet-test/a-managed-file" should contain:
      """
      a-new-local-value
      """
    And the puppet module "puppet-test" from "fakenamespace" should have 1 commit made by "Aruba" in branch "modulesync"

Does it match with your scenario?

[neomilium]

@bittner Could we close this issue in favor of #220 ?

[bittner]

I'm not sure that it covers the same topic. This here is really about a design issue: That the .sync.yml file of the target branch should be considered instead of the one of the (earlier created) topic branch.

[neomilium]

@bittner OK, I implemented what I understand from your description and it seems that now works perfectly on top of my refactoring branch.
Side effect of cleaning process or misunderstood scenario, could you have a look to it?

[neomilium]

Now fixed as tested in #242