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

config: Lead off with the purpose of the config #737

Merged
merged 1 commit into from
May 9, 2017

Conversation

wking
Copy link
Contributor

@wking wking commented Mar 16, 2017

Instead of leading off with links to a bunch of other places, notes on the Go tags, etc., make things more inviting by leading off with a big-picture summary of what the configuration is about.

Also drop the config.json existence MUST because:

  1. This section defines the configuration format, and doesn't need to be tied to a particular filename.

  2. The bundle spec (in bundle.md) already has:

    This REQUIRED file MUST reside in the root of the bundle directory and MUST be named config.json.

The config.md line may have been useful when it was added (77d44b1). But since the bundle.md line landed in #210, I think it's been redundant.

@wking wking force-pushed the config-lead-in branch 2 times, most recently from e7b00a4 to c223790 Compare March 16, 2017 22:39
@hqhq
Copy link
Contributor

hqhq commented Apr 5, 2017

LGTM

Approved with PullApprove

config.md Outdated
@@ -1,13 +1,12 @@
# <a name="containerConfigurationFile" />Container Configuration file

The container's top-level directory MUST contain a configuration file called `config.json`.
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to drop this line? Maybe you should include it below the moved paragraph?

Copy link
Contributor Author

@wking wking Apr 5, 2017

Choose a reason for hiding this comment

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

Did you mean to drop this line?

Yes, see “Also drop the config.json existence MUST because” and later. If that is contentious, I can add it back in this PR and file a follow-up PR for its removal.

Copy link
Member

Choose a reason for hiding this comment

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

this seems like a bizzare flyby. 👎

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this seems like a bizzare flyby

It didn't seem contentious to me, and it's all in the same section of the spec. But it is an orthogonal change, so I dropped it from this PR with c2237900952dd8 and filed #756 for just the redundant filename removal.

@crosbymichael
Copy link
Member

rebase plz

Instead of leading off with links to a bunch of other places, notes on
the Go tags, etc., make things more inviting by leading off with a
big-picture summary of what the configuration is about.

Signed-off-by: W. Trevor King <[email protected]>
@wking wking force-pushed the config-lead-in branch from 0952dd8 to ff071cc Compare May 9, 2017 21:13
@wking
Copy link
Contributor Author

wking commented May 9, 2017

Rebased with 0952dd8ff071cc, working around the just-landed #756 which I'd spun off this PR earlier :p.

@mrunalp
Copy link
Contributor

mrunalp commented May 9, 2017

LGTM

Approved with PullApprove

1 similar comment
@crosbymichael
Copy link
Member

crosbymichael commented May 9, 2017

LGTM

Approved with PullApprove

@crosbymichael crosbymichael merged commit 43c2154 into opencontainers:master May 9, 2017
@wking wking deleted the config-lead-in branch May 10, 2017 23:50
@vbatts vbatts mentioned this pull request Jul 5, 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.

6 participants