Skip to content

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

Merged
crosbymichael merged 1 commit into
opencontainers:masterfrom
wking:config-lead-in
May 9, 2017
Merged

config: Lead off with the purpose of the config#737
crosbymichael merged 1 commit into
opencontainers:masterfrom
wking:config-lead-in

Conversation

@wking

@wkingwking commented Mar 16, 2017

Copy link
Copy Markdown
Contributor

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
wkingforce-pushed the config-lead-in branch 2 times, most recently from e7b00a4 to c223790CompareMarch 16, 2017 22:39
@hqhq

hqhq commented Apr 5, 2017

Copy link
Copy Markdown
Contributor

LGTM

Approved with PullApprove

Comment threadconfig.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
Copy Markdown
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?

@wkingwkingApr 5, 2017

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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 <wking@tremily.us>
@wking

wking commented May 9, 2017

Copy link
Copy Markdown
ContributorAuthor

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

@mrunalp

mrunalp commented May 9, 2017

Copy link
Copy Markdown
Contributor

LGTM

Approved with PullApprove

1 similar comment
@crosbymichael

crosbymichael commented May 9, 2017

Copy link
Copy Markdown
Member

LGTM

Approved with PullApprove

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

@wking@hqhq@crosbymichael@mrunalp@vbatts@cyphar