Skip to content

Conversation

@LorbusChris
Copy link
Contributor

@LorbusChris LorbusChris commented May 15, 2020

The Ignition config specs v2.4 and v.3.1 are the currently
stable ones in Ignition 0.x and 2.x respectively.

@LorbusChris LorbusChris changed the title Translate to/from specs v2.5-exp and v.2-exp Translate to/from specs v2.5-exp and v3.2-exp May 15, 2020
@bgilbert
Copy link
Contributor

We should never output an experimental spec. There's no guarantee of compatibility even within an experimental version, and once an experimental spec is stabilized, the -experimental variant is no longer accepted by Ignition.

It might make sense to bump to 2.4 and 3.1, but we should be careful. If the converter is being used to provision OS versions which might not support those specs, we'll cause provisioning failures.

@LorbusChris LorbusChris changed the title Translate to/from specs v2.5-exp and v3.2-exp Translate to/from specs v2.4 and v3.1 May 15, 2020
@LorbusChris
Copy link
Contributor Author

LorbusChris commented May 15, 2020

I agree with the experimental part. Changed this to the stable versions.
Projects that use this library shouldn't update it use unless they support those spec versions.

Right now the most annoying with this library is however that you cannot translate to and back easily as the version expected as input to by Translate() is 2.3 and the version output by Translate3to2() is 2.2

Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

This should also add translations for new functionality that's supported in both spec versions.

Projects that use this library shouldn't update it use unless they support those spec versions.

The issue isn't whether the project supports the spec version, but whether all machines provisioned by the project also support that spec version.

@LorbusChris
Copy link
Contributor Author

I've added functionality to translate the new fields in 2.4/3.1.
This changes the main function to translate up to 3.1 and down to 2.2

Also added more individual functions for translating from/to specific versions (they might not all be required - what I'd like to use for MCO is capability to translate back and forth between 2.2 and 3.1).

@LorbusChris LorbusChris force-pushed the update-specs branch 5 times, most recently from f0ce2fa to 7fc95d0 Compare June 3, 2020 21:36
@LorbusChris
Copy link
Contributor Author

@bgilbert this is ready for review :)

@yuqi-zhang
Copy link
Contributor

The code itself seems fine, however with this PR, we would have:

  • 2.3 -> 3.0 (original)
  • 2.4 -> 3.1 (latest versions)
  • 3.0 -> 2.2 (original)
  • 3.0 -> 2.3 (not sure why we need this, I assume for compatibility?)
  • 3.1 -> 2.2 (not sure why we need this, do we want to start the MCO on 3.1 for master branch? The FCOS branch today is on 3.0)
  • 3.1 -> 2.4 (latest versions)

It feels a bit redundant. Do we need all that compatibility? The FCOS MCO branch today is on 3.0. Are we able to convert between 3.0 to 3.1? If so I think we can drop some of the excessive translations.

@LorbusChris
Copy link
Contributor Author

LorbusChris commented Jun 5, 2020

yep, the intention is to go straight to spec 3.1 in OCP (I.e. openshift/machine-config-operator#1778, after the dual support PR has landed and propagated through to MCD on RHCOS)

I think I liked the idea of adding 3.0 -> 2.3 so we can translate back and forth between them, but it's not really necessary indeed. I'll remove that one.

@yuqi-zhang
Copy link
Contributor

Right, hopefully after OCP stabilizes and is able to remove spec 2.x entirely, we would no longer need to support translations (unless some other major case shows up). So we won't have to add more backward compatible versions in the future. Does that make sense?

@LorbusChris
Copy link
Contributor Author

Dropped the 3.0 -> 2.3 conversion func

@LorbusChris
Copy link
Contributor Author

@bgilbert I believe I've addressed all comments. PTAL

@LorbusChris LorbusChris force-pushed the update-specs branch 3 times, most recently from e9aeae9 to 4d80642 Compare June 25, 2020 23:48
Also moves some common functions into the util package.
Also fixes the Ignition configs used for testing to adhere to this rule.
@LorbusChris LorbusChris force-pushed the update-specs branch 2 times, most recently from 5c08a0b to 8bfca33 Compare June 26, 2020 14:17
@LorbusChris
Copy link
Contributor Author

@bgilbert I've pushed the next iteration of this :)

Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

🎉

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

Successfully merging this pull request may close these issues.

3 participants