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

compose: Factor out helper for writing composejson #1636

Closed
wants to merge 2 commits into from

Conversation

cgwalters
Copy link
Member

Prep for sharing this code with rojig.

Prep for sharing this code with rojig.

if (opt_write_composejson_to)
if (opt_write_commitid_to)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think this slightly changes the semantics of --write-commitid-to though. Before it would override ref, but now we're doing both. That's likely to cause issues in e.g. pungi, which does the ref update as a separate step.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have a test for that:

# --write-commitid-to should not set the ref
if ostree --repo=${repobuild} rev-parse ${treeref}; then
    fatal "Found ${treeref} ?"
fi
echo "ok ref not written"

which is passing. I think the diff is just misleading. If you look at the logic above, we're still doing:

  /* --write-commitid-to overrides writing the ref */
  if (self->ref && !opt_write_commitid_to)
    {
      if (use_txn)
        ostree_repo_transaction_set_ref (self->repo, NULL, self->ref, new_revision);
      else
        {
          if (!ostree_repo_set_ref_immediate (self->repo, NULL, self->ref, new_revision,
                                              cancellable, error))
            return FALSE;
        }
    }

Copy link
Member

Choose a reason for hiding this comment

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

Ahh yeah, you're right. I misread it. Although, this chunk now is no longer overridden by --write-commitid-to, right?

if (self->ref)
  {
     g_print ("%s => %s\n", self->ref, new_revision);
     g_variant_builder_add (&composemeta_builder, "{sv}", "ref", g_variant_new_string (self->ref));
  }

So now, we're always printing that => line and inserting the ref key in the JSON even if we didn't actually update the ref, which is a bit misleading.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, updated ⬇️

@jlebon
Copy link
Member

jlebon commented Oct 26, 2018

@rh-atomic-bot r+ 84d2275

@cgwalters
Copy link
Member Author

@rh-atomic-bot retry

@cgwalters
Copy link
Member Author

@rh-atomic-bot r=jlebon 84d2275

@jlebon
Copy link
Member

jlebon commented Oct 29, 2018

@rh-atomic-bot r+

@jlebon jlebon closed this Oct 29, 2018
@jlebon jlebon reopened this Oct 29, 2018
@rh-atomic-bot
Copy link

⌛ Testing commit 84d2275 with merge b5d34b7...

@rh-atomic-bot
Copy link

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing b5d34b7 to master...

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