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

Use ksonnet to generate examples #9

Closed
wants to merge 3 commits into from
Closed

Use ksonnet to generate examples #9

wants to merge 3 commits into from

Conversation

chuckha
Copy link
Contributor

@chuckha chuckha commented Jul 30, 2017

Closes issue #5.

  • Adds jsonnet to generate the quickstart examples
  • Removes the YAML
  • Adds the generated JSON
  • Docs are updated to reflect JSON files vs YAML
  • Adds a Makefile target, generate-examples

Signed-off-by: Chuck Ha chuck@heptio.com

@chuckha chuckha requested a review from timothysc July 30, 2017 21:35
@chuckha
Copy link
Contributor Author

chuckha commented Jul 30, 2017

Here is the process I took to ensure the generated json is equivalent to yaml:

  1. Generate json with new make command
  2. Convert generated json to yaml (https://github.com/redsymbol/json2yaml)
  3. YAML diff (https://www.npmjs.com/package/koimo)
cha$ node_modules/.bin/koimo gen_00-rbac.yaml 00-rbac.yaml 
  
  ===================
   Koimo - YAML Diff 
  ===================
  
  [ gen_00-rbac.yaml ] vs. [ 00-rbac.yaml ]

  ✓ All good!

cha$ node_modules/.bin/koimo gen_10-configmaps.yaml 10-configmaps.yaml 
  
  ===================
   Koimo - YAML Diff 
  ===================
  
  [ gen_10-configmaps.yaml ] vs. [ 10-configmaps.yaml ]

  ✓ All good!

cha$ node_modules/.bin/koimo gen_20-pod.yaml 20-pod.yaml 
  
  ===================
   Koimo - YAML Diff 
  ===================
  
  [ gen_20-pod.yaml ] vs. [ 20-pod.yaml ]

  ✓ All good!

@chuckha
Copy link
Contributor Author

chuckha commented Jul 31, 2017

I would love input on what to do with the YAML. I'm not convinced deleting it is the best path forward because in my experience people would prefer reading YAML. If they are looking for a way to get started, it would be nice to have the YAML available.

Copy link

@kensimon kensimon left a comment

Choose a reason for hiding this comment

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

Small fixes, plus maybe want @hausdorff to review the jsonnet (I'm a little out of my expertise here)

Makefile Outdated
$(MAKE) -C build/systemd-logs clean
rm -f /examples/quickstart/*.json

Choose a reason for hiding this comment

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

Probably don't want to rm -r /examples, but ./examples?


The JSON files are generated with the `make generate-examples` command. To get this to work you must have ksonnet-lib installed in `/usr/local/lib/jsonnet/ksonnet-lib`. After that, `make generate-examples` will build the JSON files that are checked into this repository.

0: https://ksonnet.heptio.com

Choose a reason for hiding this comment

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

HTTPS doesn't work on this site, link needs fixing

}
},
{
"apiVersion": "rbac.authorization.k8s.io/v1beta1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we somehow option wrap rbac with a default of true but allow for disabled generation per other issue filed?

Choose a reason for hiding this comment

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

It's not easy right now, but it's certainly on the roadmap for the CLI.

One way to do it right now is to use ext vars, which are passed in via command line. Your build process would involve interrogating the API server, getting capabilities, and passing a flag --ext-str with a value to indicate as such.

{
"apiVersion": "v1",
"data": {
"config.json": "{\"Description\": \"EXAMPLE\", \"Filters\": {\"LabelSelector\": \"\", \"Namespaces\": \".*\"}, \"PluginNamespace\": \"heptio-sonobuoy\", \"Plugins\": [{\"name\": \"systemd_logs\"}, {\"name\": \"e2e\"}], \"Resources\": [\"CertificateSigningRequests\", \"ClusterRoleBindings\", \"ClusterRoles\", \"ComponentStatuses\", \"Nodes\", \"PersistentVolumes\", \"PodSecurityPolicies\", \"ServerVersion\", \"StorageClasses\", \"ThirdPartyResources\", \"ConfigMaps\", \"DaemonSets\", \"Deployments\", \"Endpoints\", \"Events\", \"HorizontalPodAutoscalers\", \"Ingresses\", \"Jobs\", \"LimitRanges\", \"PersistentVolumeClaims\", \"Pods\", \"PodLogs\", \"PodDisruptionBudgets\", \"PodPresets\", \"PodTemplates\", \"ReplicaSets\", \"ReplicationControllers\", \"ResourceQuotas\", \"RoleBindings\", \"Roles\", \"ServiceAccounts\", \"Services\", \"StatefulSets\"], \"ResultsDir\": \"/tmp/sonobuoy\", \"Server\": {\"advertiseaddress\": \"sonobuoy-master:8080\", \"bindaddress\": \"0.0.0.0\", \"bindport\": 8080, \"timeoutseconds\": 3600}, \"Version\": \"v0.8.0\"}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This almost makes it unreadable...

Choose a reason for hiding this comment

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

I would use kubecfg here, and generate YAML instead of JSON. kubecfg natively emits multi-line strings all prettified.

}
},
{
"apiVersion": "rbac.authorization.k8s.io/v1beta1",

Choose a reason for hiding this comment

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

It's not easy right now, but it's certainly on the roadmap for the CLI.

One way to do it right now is to use ext vars, which are passed in via command line. Your build process would involve interrogating the API server, getting capabilities, and passing a flag --ext-str with a value to indicate as such.


};

local namespace = local ns = k.core.v1.namespace;

Choose a reason for hiding this comment

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

Super tiny micro nit, when jsonnet fmt matures, this local will get indented below. AFAIK that's actively being worked on by Dave's intern.

{
"apiVersion": "v1",
"data": {
"config.json": "{\"Description\": \"EXAMPLE\", \"Filters\": {\"LabelSelector\": \"\", \"Namespaces\": \".*\"}, \"PluginNamespace\": \"heptio-sonobuoy\", \"Plugins\": [{\"name\": \"systemd_logs\"}, {\"name\": \"e2e\"}], \"Resources\": [\"CertificateSigningRequests\", \"ClusterRoleBindings\", \"ClusterRoles\", \"ComponentStatuses\", \"Nodes\", \"PersistentVolumes\", \"PodSecurityPolicies\", \"ServerVersion\", \"StorageClasses\", \"ThirdPartyResources\", \"ConfigMaps\", \"DaemonSets\", \"Deployments\", \"Endpoints\", \"Events\", \"HorizontalPodAutoscalers\", \"Ingresses\", \"Jobs\", \"LimitRanges\", \"PersistentVolumeClaims\", \"Pods\", \"PodLogs\", \"PodDisruptionBudgets\", \"PodPresets\", \"PodTemplates\", \"ReplicaSets\", \"ReplicationControllers\", \"ResourceQuotas\", \"RoleBindings\", \"Roles\", \"ServiceAccounts\", \"Services\", \"StatefulSets\"], \"ResultsDir\": \"/tmp/sonobuoy\", \"Server\": {\"advertiseaddress\": \"sonobuoy-master:8080\", \"bindaddress\": \"0.0.0.0\", \"bindport\": 8080, \"timeoutseconds\": 3600}, \"Version\": \"v0.8.0\"}"

Choose a reason for hiding this comment

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

I would use kubecfg here, and generate YAML instead of JSON. kubecfg natively emits multi-line strings all prettified.

};

local plugins = {
"systemdlogs.yaml": 'name: systemd_logs

Choose a reason for hiding this comment

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

Hmm, it seems to me you could write this as a normal JSON object and then call std.toString(systemdLogs) on it to get a string? Am I missing something crucial?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately the plugin loader only knows how to read YAML.

I'll make an issue and this issue will depend on that issue.

Choose a reason for hiding this comment

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

@chuckha Is YAML not a superset of JSON, though? So it seems like we should be able to use JSON here and be ok.

If not, kubecfg allows you to import YAML files, as well manifestYAML, which pretty-prints an object as YAML.

Or perhaps I am missing something?

k.core.v1.list.new([
sonobuoyConfig,
pluginConfigs,
])

Choose a reason for hiding this comment

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

Tiny, tiny microest of nits, you might consider putting newlines at the end so that diffs are cleaner.

@timothysc
Copy link
Contributor

So I'm inclined to hold off on this unless it is strictly cleaner and more concise, that is the bar we should be holding here.

@hausdorff
Copy link

@timothysc Your comment here makes me wonder if there is anything beyond the comments listed you'd like to see changed? Changing to YAML will make it cleaner and more concise, but it's not a change to the Jsonnet code itself, which is where I'd expect the bulk of cleanliness changes to go.

@timothysc
Copy link
Contributor

@hausdorff Right now I think the biggest detractor is the readability. I'd also like to get to the point where we can generate the plugin definitions, and perhaps use that as a mixin to the quickstart .yaml.

@hausdorff
Copy link

@timothysc Strongly agree about the plugin definitions, which I think are doable with std.toString, I think I mentioned this before.

I think my main question, though, is that it's not clear (to me, at least) what else needs to change to meet the readability bar. Just the string pretty printing?

@timothysc
Copy link
Contributor

I think my main question, though, is that it's not clear (to me, at least) what else needs to change to meet the readability bar. Just the string pretty printing?

Eliminate the nested structure string escaping.

@chuckha chuckha mentioned this pull request Aug 10, 2017
Chuck Ha added 3 commits August 14, 2017 12:24
Closes issue #5.

- Adds jsonnet to generate the quickstart examples
- Removes the YAML
- Adds the generated JSON
- Docs are updated to reflect JSON files vs YAML
- Adds a Makefile target, `generate-examples`

Signed-off-by: Chuck Ha <chuck@heptio.com>
@chuckha
Copy link
Contributor Author

chuckha commented Aug 17, 2017

I believe all issues have been addressed in #52

@chuckha chuckha closed this Aug 17, 2017
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.

None yet

4 participants