Skip to content
This repository has been archived by the owner on Nov 20, 2023. It is now read-only.

Support json as config as well as yaml #355

Closed
wants to merge 6 commits into from
Closed

Support json as config as well as yaml #355

wants to merge 6 commits into from

Conversation

spboyer
Copy link
Contributor

@spboyer spboyer commented Apr 10, 2020

Addresses #15

tye.sln Outdated Show resolved Hide resolved
string yamlContent = serializer.Serialize(obj);

using var parser = new YamlParser(yamlContent);
return parser.ParseConfigApplication();
Copy link
Member

Choose a reason for hiding this comment

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

Is this step even needed? (would YamlParser just parse the JSON document since JSON is YAML).

I'm thinking about things like line-numbers matching up if we do a conversion step. We just did a bunch of work in the yaml parser/validator to get better error messages with line numbers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, would have to test. First though is no. Yaml is format (space) specific where JSON can be one line no spaces and still be valid.

@@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using k8s;
Copy link
Member

Choose a reason for hiding this comment

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

Does this class actually use the k8s OM for anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, removed.

{
if (path is FileInfo && path.Exists && !force)
{
ThrowIfTyeFilePresent(path, "tye.yml");
ThrowIfTyeFilePresent(path, "tye.yaml");

if (json)
ThrowIfTyeFilePresent(path, "tye.json");
Copy link
Member

Choose a reason for hiding this comment

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

This check should probably happen regardless of json being true/false. We don't want to create a tye.yaml if you already have tye.json without force.

@@ -77,7 +88,7 @@ public static (string, string) CreateTyeFileContent(FileInfo? path, bool force)
}

// If the input file is a sln/project then place the config next to it
outputFilePath = Path.Combine(directory.FullName, "tye.yaml");
outputFilePath = Path.Combine(directory.FullName, outputFilePath);
Copy link
Member

Choose a reason for hiding this comment

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

What does this do with our comments? https://github.com/dotnet/tye/pull/355/files#diff-03e018baedc83b3a7dd33c431001ccfdR48 when you do JSON?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comments are not supported in JSON. Also consider the following note from http://www.yaml.org/spec/1.2/spec.html section 3.2.3.3. Comments

Comments are a presentation detail and must not have any effect on the serialization tree or representation graph...


var json = serializer.Serialize(yamlObject);

return json;
Copy link
Member

Choose a reason for hiding this comment

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

It would be really nice if this could also inject $schema pointing at: https://raw.githubusercontent.com/dotnet/tye/master/src/schema/tye-schema.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added.

@spboyer spboyer marked this pull request as ready for review April 15, 2020 14:45
@spboyer spboyer requested a review from jkotalik as a code owner April 15, 2020 14:45
@spboyer
Copy link
Contributor Author

spboyer commented May 1, 2020

This was a good spike. Moving away in favor of current configuration option,
@davidfowl

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants