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

Safe versioning of .braids.json #66

Closed
mattmccutchen opened this issue Jun 8, 2017 · 12 comments
Closed

Safe versioning of .braids.json #66

mattmccutchen opened this issue Jun 8, 2017 · 12 comments
Assignees

Comments

@mattmccutchen
Copy link
Collaborator

Each time a new version of Braid changes the .braids.json format in a way that older versions of Braid will process incorrectly (e.g., e6535aa), teams can get incorrect behavior unless they manually communicate and make sure that everyone who uses Braid upgrades it at the same time. We should add a version number to the configuration file and add a check to Braid that the version number is not newer than the version it understands.

Then, ideally, we'd make a one-time change to the configuration file to block out all versions of Braid from before the version check was added. Unfortunately, if we just rename it, then old versions of Braid will behave as if no mirrors are defined; some commands will fail with "mirror does not exist", but braid add will happily create a new configuration file, which will just lead to more confusion. The only way to reliably cause old versions of Braid to fail is to deliberately make .braids.json invalid JSON, but that's a pain for anyone who wants to process it with other tools. And if we wanted to block out versions of Braid from before the .braids.json name was introduced, we'd have to create a .braids file. This becomes ugly. Maybe the most reasonable thing is to forget about this and just tell teams to upgrade everyone to at least Braid 1.0.22, and then after that point, they will be notified when they need to upgrade.

@realityforge
Copy link
Collaborator

A version flag in the braids file does seem reasonably sensible. How we handle previous versions basically comes down to how much time is spent to supporting the older versions. I believe the .braids => .braids.json should be reasonably easy to do (And may be in place IIRC). The presence of "squashed": false, could generate an exception so people are warning that the old behaviour is no longer supported. "lock": "X", could be translated to "revision": "X", and removal of "branch" key ... so in theory support could go back until braid stopped supporting subversion etc.

@mattmccutchen mattmccutchen self-assigned this Jun 8, 2017
@mattmccutchen
Copy link
Collaborator Author

How we handle previous versions basically comes down to how much time is spent to supporting the older versions.

You're talking about a new versions of Braid migrating an old configuration. I filed this issue about the opposite scenario: an old version of Braid that sees a new configuration that it doesn't understand should raise an error and tell the user to upgrade rather than doing the wrong thing.

Re the particular cases of new versions of Braid migrating old configurations:

I believe the .braids => .braids.json should be reasonably easy to do (And may be in place IIRC).

Yes, that already works.

The presence of "squashed": false, could generate an exception so people are warning that the old behaviour is no longer supported.

Based on this comment I thought you wanted to change the behavior without warning. It's a little late now, but if you're OK with making this an error, I'll go back and do that in #56.

"lock": "X", could be translated to "revision": "X", and removal of "branch" key

That's precisely #65.

@realityforge
Copy link
Collaborator

Ahh - I see. Yes - not sure how we can gracefully handle forward compatibility for older braids. Other than as you suggest -adding the a version flag and starting with forward compatibility from now onwards.

@mattmccutchen
Copy link
Collaborator Author

Another thought (outside the original scope of this issue but convenient to discuss here):

Should we prompt the user before upgrading the configuration, just on the basis that other users on the project may then have to upgrade Braid, even if no incompatible behavior changes (such as converting full-history mirrors to squashed) are being made? Or do we assume that it's no problem for other users to upgrade Braid on demand (using gem if they were using a third-party package that hasn't been updated yet), so such a prompt wouldn't be helpful?

I'm leaning against prompting. Is there any precedent we should consider among multi-user applications? (I know lots of single-user applications that assume the single user won't downgrade the application and thus upgrade data without prompting.)

@realityforge
Copy link
Collaborator

I don't have a strong opinion either way and see merits with both approaches.

The braid upgrade is by far the "safer", more "user friendly" approach as it forces people to acknowledge that they are adding a new dependency on their project. It probably also has more complex code as we need new task to do the upgrade and we also presumably allow later variants of braid to work with earlier versions of configuration and continue to write out earlier version of configuration if possible. So we get more code to maintain but users get a more pleasant experience.

The "auto upgrade" approach is probably less work for us to maintain (only have to emit a single form of output and we can transform config during load). However it makes it harder for users to control dependencies on versions.

@mattmccutchen
Copy link
Collaborator Author

continue to write out earlier version of configuration if possible

I agree that would be complicated and I don't want to do it now. We can still ask the user before upgrading the configuration. I've already started drafting the code for a braid upgrade-config command (braid upgrade is too easy to confuse with braid update), because we definitely want an explicit command for incompatible behavior changes, and it's easy to make Braid raise an error and require the user to run this command for compatible upgrades as well as incompatible ones.

Hopefully most users will keep Braid up to date and it won't be a big deal. If some users have more constraints on versioning, they may have to go to some trouble (e.g., manually installing multiple versions of Braid), but at least we are warning them about what they're getting into.

Expanding this issue to cover both the check for a configuration that is too old (and braid upgrade-config flow) and the check for a configuration that is too new.

@mattmccutchen mattmccutchen changed the title Add version to .braids.json so Braid can check if it's too old to process .braids.json correctly Safe versioning of .braids.json Jun 9, 2017
@mattmccutchen
Copy link
Collaborator Author

mattmccutchen commented Dec 28, 2017

I have a more detailed design proposal. @realityforge, may I have your feedback before I proceed with implementation?

This will be a considerable amount of work. I'm excited to get started on it while I'm on vacation. I'm not sure when or if I will finish, but even if I don't, the next person will have a plan they can follow if they like it.

Goals

Prevent surprises and adequately support normal development processes while minimizing the additional maintenance cost of the version compatibility mechanism. I'd like to have Braid in good enough shape (on this issue as well as others) that I can confidently recommend it to teams whose use of Braid might outlive my availability to help them with it; I'm thinking of the team I worked with at my internship this past summer as an example. However, I don't want to spend extra work now and commit to extra future maintenance cost for non-essential features unless/until there's evidence of demand for them.

Overview

  • Configuration format:
{
  "config_version": 1,
  "braids": { ... }
}
  • config_version is a positive integer that is increased every time we change the configuration semantics.
  • Assume for now that each Braid version has a preferred config version and is able to upgrade older versions modulo some breaking changes, but there is no support for writing out older versions.
    • Nothing prevents us from choosing to have future versions of Braid support writing more than one config version.
    • The preferred config version does not change within a minor semantic version (x.y) of Braid. We can release critical bug fixes within a minor version, and users can always upgrade Braid to the latest patch within a minor version without configuration compatibility implications.
    • The mapping between Braid versions and config versions is posted on a web page under https://cristibalan.github.io/braid/ . The page includes instructions for running gem install and using the braid _VERSION_ syntax to invoke a particular version of Braid.
    • In theory, every config version increase could be argued to be a breaking change (removal of support for writing the previous config version) that would merit a major version bump. I'll argue that would be too ugly and it isn't important to follow semantic versioning strictly because Braid is intended for interactive use and we detect all problems and advise the user of what to do. People can always pin the minor version if they really want to.
  • General Braid command behavior:
    • If config version is too new:
      • Loading the config raises an error: "Braid: Cannot perform this command because this version of Braid (x.y.z) is too old to understand your configuration file (version N). Run gem update braid to try the latest version of Braid, or check URL for the latest Braid version that supports configuration version N."
    • If config version is too old:
      • Loading the config automatically upgrades it in memory, logging any breaking changes.
      • For any command except braid upgrade-config itself, raise an error at the end of config loading if there are breaking changes: "Braid: Cannot perform this command because this version of Braid (x.y.z) no longer supports a feature used by your configuration file (version N). Run braid upgrade-config --dry-run for details, or check URL for the latest Braid version that supports configuration version N."
      • A command that normally writes to the config raises an error at the end of config loading: "Braid: Not performing this command with this version of Braid (x.y.z) because it would require upgrading your configuration file (current version N), which may force other developers on your project to upgrade Braid. Run braid upgrade-config to proceed with the upgrade, or check URL for the latest Braid version that supports configuration version N."
  • Config upgrading:
    • braid upgrade-config --dry-run: general explanation of what's going to happen, and list any breaking changes.
    • braid upgrade-config: if there are no breaking changes, do it and commit the config file
    • braid upgrade-config --allow-breaking-changes: same, but even if there are breaking changes
    • In the future, consider adding a setting to automatically make upgrades if there are no breaking changes. This would be useful if all developers of the project use a centrally managed environment with a consistent version of Braid.
      • Environment variable, per-project configuration setting, or what?
  • Transition plan
    • Release the versioning redesign in the same release as the next change to configuration semantics, hopefully Support a mirror consisting of a single file #64. Fix Braid doesn't honor revision locks from Braid < 1.0.18 #65 before then. Add detection of full-history mirrors as a breaking change (Full-history mirrors are broken: trying to use a tree as a commit #56) along with the versioning redesign.
    • Braid versions after the redesign will treat a configuration file without config_version as an old configuration version and will be able to upgrade it.
    • Braid versions from 1.0.9 up to the redesign, faced with a new-format configuration file, in most cases will hit an uncaught exception when they try to parse config_version as a mirror. I think that's close as we can get to the ideal of printing the same error as a post-redesign Braid facing a too-new configuration file.
    • Braid versions prior to 1.0.9, faced with a project using .braids.json pre or post the redesign, will ignore it and create a .braids file. I think it would be too ugly to create a dummy .braids file just to block out those versions.

History of configuration changes (for reference)

  • Braid 1.0.18
    • Locked mirrors indicated by absence of "branch" and "tag" attributes, not presence of "lock" attribute (e6535aa).
  • Braid 1.0.17
    • Support for full-history mirrors ("squashed": false) removed; "squashed" attribute no longer written (eb72030).
  • Braid 1.0.11
    • "remote" attribute no longer written (f8fd088). No special handling needed for upgrade: the attribute will just be dropped.
  • Braid 1.0.9
    • .braids -> .braids.json (6806c61).
  • Braid 1.0.0

Test cases to write

(Note: The single "breaking change" we have to work with right now is removal of full-history mirrors.)

  • Test that a read-only command:
    • When the config is too new: fails with the appropriate message
    • When the config is too old and there's a breaking change: fails with a message about the breaking change
    • When the config is too old and there's no breaking change: works
  • Test that a writing command:
    • When the config is too new: fails with the appropriate message (this takes the same code path as a read-only command, but another test case doesn't hurt)
    • When the config is too old and there's a breaking change: fails with a message about the breaking change
    • When the config is too old and there's no breaking change: fails with a message about forcing other developers to upgrade Braid
  • Test upgrades from all the supported old formats and verify the configuration file ends up with the correct content.
  • Test that braid upgrade-config --allow-breaking-changes does the right thing:
    • When already at the preferred config version: no-op
    • With no breaking change: upgrade
    • With a breaking change: upgrade
  • Test that braid upgrade-config does the right thing:
    • When already at the preferred config version: no-op
    • With no breaking change: failure
    • With a breaking change: upgrade
  • Test that braid upgrade-config --dry-run gives correct explanations:
    • When already at the preferred config version
    • With no breaking change
    • With a breaking change

@realityforge
Copy link
Collaborator

Looks good to me. The only nitpick that jumped out at me is that the key should probably follow the same conventions as the rest of our keys. i.e. camelCase "configVersion" or maybe just "version".

I would also tend to try to perform a graceful upgrade from our current config version to the versioning config version. You could just assume that a lack of version key means it is in current version config and that would cover 95% of the use case I would guess.

Thanks for moving this forward.

@mattmccutchen
Copy link
Collaborator Author

The only nitpick that jumped out at me is that the key should probably follow the same conventions as the rest of our keys. i.e. camelCase "configVersion" or maybe just "version".

It doesn't look like we have any precedent for multi-word keys, but multi-word variables in the source code are lowercase with underscores, so I'll go with that.

I would also tend to try to perform a graceful upgrade from our current config version to the versioning config version. You could just assume that a lack of version key means it is in current version config and that would cover 95% of the use case I would guess.

I forgot to explain that part. I added a brief point to the overview above. In a bit more detail: So far, all of our configuration upgrades have been written in an idempotent way, so Braid has just tried all of them to upgrade a configuration file from any previous version. Keeping with that approach, post-redesign Braid will be able to upgrade any pre-redesign configuration file with braid upgrade-config. This upgrade (like any other under the new design) will not be automatic because we want to make sure the user accepts that other developers on the project will have to upgrade Braid to understand the upgraded configuration file. Once we have the config_version key (the primary purpose of which is to ensure that old Braid versions refuse to work on new configuration files even if the files just gain new semantics without changing format, as in #64), it will be up to us whether we keep future configuration upgrades idempotent or take advantage of the config_version key to conditionally perform non-idempotent upgrades.

@mattmccutchen
Copy link
Collaborator Author

The mapping between Braid versions and config versions is posted on a web page under https://cristibalan.github.io/braid/ .

@realityforge, can you help me figure out the right way to do this? It looks like the current content of the gh-pages branch was generated from README.md of an old revision of master. I probably want to add another page generated from Markdown using the same theme, or conceivably just add a section to README.md and use a link with a fragment identifier. Are we using the GitHub Pages support for Jekyll (or should we be — c.f. #67), or is there a manual process to regenerate gh-pages?

Might this be a good opportunity to transfer the Braid repository to a GitHub organization so you can have repository admin privileges and the URL will be stable if cristibalan needs to completely give up involvement? cristibalan can remain the only owner for now if he wants. The name braid is taken, but we could consider braid-vc (abbreviation for "version control") or braid-vendoring.

I'll continue to work on the code while you sort this out.

mattmccutchen added a commit to mattmccutchen/braid that referenced this issue Jan 1, 2018
update".

This is in preparation for supporting single-file mirrors, which will
have a different data structure in place of the tree ID.

- The check if the mirror is already up to date: I believe the change
  makes no difference because the only way to get here with switching ==
  false and was_locked == true is if the old and new upstream revision
  are equal (it's not enough if they have equal content at the remote
  path).  I'd like to clean up this code, but that becomes an invasive
  change that I don't want to block cristibalan#64 and cristibalan#66 on.
- The label in conflict markup: This seems to make sense.
mattmccutchen added a commit to mattmccutchen/braid that referenced this issue Jan 1, 2018
(DO NOT MERGE YET: needs the version table web page to be written.)

Fixes cristibalan#66.

Now that we have the infrastructure:
- Report loss of support for full-history mirrors as a breaking change.
  Fixes cristibalan#56.
- Correctly upgrade revision locks from Braid < 1.0.18.  Fixes cristibalan#65.
mattmccutchen added a commit to mattmccutchen/braid that referenced this issue Jan 1, 2018
Bump version to 1.1.0 according to the new policy that each
configuration version corresponds to a different Braid minor version.

(DO NOT MERGE YET: needs the version table web page to be written.)

Fixes cristibalan#66.

Now that we have the infrastructure:
- Report loss of support for full-history mirrors as a breaking change.
  Fixes cristibalan#56.
- Correctly upgrade revision locks from Braid < 1.0.18.  Fixes cristibalan#65.
mattmccutchen added a commit to mattmccutchen/braid that referenced this issue Jan 1, 2018
Bump version to 1.1.0 according to the new policy that each
configuration version corresponds to a different Braid minor version.

(DO NOT MERGE YET: needs the version table web page to be written.)

Fixes cristibalan#66.

Now that we have the infrastructure:
- Report loss of support for full-history mirrors as a breaking change.
  Fixes cristibalan#56.
- Correctly upgrade revision locks from Braid < 1.0.18.  Fixes cristibalan#65.
@realityforge
Copy link
Collaborator

Sorry for taking a while to get back to you - I have a 4-week year old that is taking up most of my time ;)

Anyhoo - the way I have done this in the past was actually to fork this repo and regenerating the website on the fork from the GitHub website and then merging/PR-ing into the main repo the output. The other option I have tried is just manually patching the html. I assume those options would still work. We could also use jekyll directly but that seems like a smidge more work. HTH

@mattmccutchen
Copy link
Collaborator Author

I have a 4-week year old that is taking up most of my time ;)

Congratulations!

I researched the web site situation some more. It's currently in legacy Automatic Page Generator mode, but even without access to the settings of this repository, we can switch it to the new mode (in which GitHub automatically converts the Markdown to HTML and applies the theme invisibly to us) by going through GitHub's wizard on a fork and pushing the resulting content to the publishing source branch (currently gh-pages) of this repository. However, this would be a good opportunity to fix #67 by switching the source to the master branch so I can add my content there rather than to the gh-pages branch. I've emailed cristibalan to ask him to do that and also to inquire about transferring the repository to an organization.

mattmccutchen added a commit to mattmccutchen/braid that referenced this issue Feb 18, 2018
Bump version to 1.1.0 according to the new policy that each
configuration version corresponds to a different Braid minor version.

Fixes cristibalan#66.

Now that we have the infrastructure:
- Report loss of support for full-history mirrors as a breaking change.
  Fixes cristibalan#56.
- Correctly upgrade revision locks from Braid < 1.0.18.  Fixes cristibalan#65.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants