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

[Master] generate/generate: Fix "specifed" -> "specified" typo #258

Merged

Conversation

Mashimiao
Copy link

And a "priviledge" -> "privilege" typo. Both turned up by Misspell
1 and reported by Go Report Card 2.

Backported to master from #251

Signed-off-by: W. Trevor King wking@tremily.us
Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com

And a "priviledge" -> "privilege" typo.  Both turned up by Misspell
[1] and reported by Go Report Card [2].

[1]: https://github.com/client9/misspell
[2]: https://goreportcard.com/report/github.com/opencontainers/runtime-tools

Backported to master from opencontainers#251

Signed-off-by: W. Trevor King <wking@tremily.us>
Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
@Mashimiao Mashimiao changed the title generate/generate: Fix "specifed" -> "specified" typo [Master] generate/generate: Fix "specifed" -> "specified" typo Oct 25, 2016
@vbatts
Copy link
Member

vbatts commented Oct 25, 2016

LGTM

@wking
Copy link
Contributor

wking commented Oct 25, 2016

On Tue, Oct 25, 2016 at 02:59:45AM -0700, Ma Shimiao wrote:

Backported to master from #251

I'd rather just merge the 1.0.0.rc1 branch into master (#181).
Cherry-picking everything that's not post-1.0.0-rc1 specific at a
per-commit level seems like twice the work for no gain.

@Mashimiao
Copy link
Author

Actually, I also don't want to do this.
I want this bug to be fixed and you have fixed this in v1.0.0.rc1 branch.
The master branch should have the latest code.
If a bug is general, we should not fix it in v1.0.0.rc1 first, should fix in master branch.
v1.0.0.rc1 should backports from master, master should not backports from v1.0.0.rc1.
@opencontainers/runtime-tools-maintainers

@wking
Copy link
Contributor

wking commented Oct 26, 2016

On Tue, Oct 25, 2016 at 06:56:46PM -0700, Ma Shimiao wrote:

The master branch should have the latest code.

Agreed.

If a bug is general, we should not fix it in v1.0.0.rc1 first,
should fix in master branch. v1.0.0.rc1 should backports from
master, master should not backports from v1.0.0.rc1.

I'd rather have neither branch contain many cherry-picks. I
recommended targetting most new PRs at the stable branch
(e.g. 1.0.0.rc1) and periodically merging that branch into master to
keep master current 1. Since I proposed that in July, GitHub has
added a UI for changing a pull request's base branch [2,3], so
maintainers can easily handle poor contributor targetting. This gives
us an easy way to maintain both the master and stable branches
without having to file a duplicate back-/forward-port PR for every
change.

@mrunalp
Copy link
Contributor

mrunalp commented Oct 26, 2016

@wking I think that most developers are used to fixing master and then backporting to stable. Hence your proposal isn't gaining traction.

@wking
Copy link
Contributor

wking commented Oct 27, 2016

On Wed, Oct 26, 2016 at 02:34:08PM -0700, Mrunal Patel wrote:

I think that most developers are used to fixing master and then
backporting to stable.

I'm pointing out what I think would be an easier workflow and
addressing any misconceptions (e.g. “initially targetting the stable
branch means that master will fall behind”) that I hear about the
workflow I'm advocating. If those arguments aren't convincing, you're
obviously free to use whatever workflow makes sense to you.

@Mashimiao
Copy link
Author

I think most of us have agreed to fix master and then backport to stable.
So, should we let this PR pass. If not, I suppose we have to make a new PR to fix it.
ping @opencontainers/runtime-tools-maintainers

@hqhq
Copy link
Contributor

hqhq commented Nov 4, 2016

LGTM

I definitely missed some historical background, I'm wondering why we have that stable branch in runtime-tools in the first place with the fact that we don't even have stable branch in spec and runtime. In my understanding, tools should be kind of reflect or follow up of spec and runtime. And AFAICS, runtime-tools is still under heavily development as well.

Enlighten me if I'm wrong.

Approved with PullApprove

@Mashimiao
Copy link
Author

@hqhq all I can got is here

@wking
Copy link
Contributor

wking commented Nov 4, 2016

On Thu, Nov 03, 2016 at 08:00:27PM -0700, Qiang Huang wrote:

… I'm wondering why we have that stable branch in runtime-tools in
the first place with the fact that we don't even have stable branch
in spec and runtime.

The spec tags releases, which is as close as specs get to stable
branches ;). runtime-tools coverage is (still) not complete, and a
stable branch allows for improved coverage of at least one released
spec in parallel with bleeding-edge-spec work 1. More background
discussion in:

And in a somewhat related vein:

  • Use Python's unittest for validating bundles and configurations? #98, where I suggest Python's unittest as a convenient way to
    support validation for multiple spec versions simultaneously. If we
    had something like this (supporting multiple spec versions with one
    codebase), we wouldn't need separate per-spec-release branches.

    I've also suggested a similar non-Go test harness for runtime
    validation 3, although you'd need to translate older configs to
    something the bleeding-edge-spec-only runtimetest understood (for
    tests based on runtimetest, you wouldn't need spec translation where
    you could use BusyBox to perform the in-container portion of the
    test).

@hqhq
Copy link
Contributor

hqhq commented Nov 4, 2016

I get it if some test cases work for rc1 and be broken in rc2, it's because we're not able to follow the normal rc rules (in rc releases, take bugfixes only) in runtime-spec and runc.

And I still don't see any reasons why should tools stick to rc1 if we are moving forward to rc2 or final 1.0.0. In other words, why do we want validation for multiple spec versions simultaneously before 1.0.0? It looks not needed to me, not to mention that we've already been short of hands on this project.

Such kind of branch only makes sense after 1.0.0, as a LTS. So I suggest we keep focus on master branch for now.

@wking
Copy link
Contributor

wking commented Nov 4, 2016

On Thu, Nov 03, 2016 at 11:42:42PM -0700, Qiang Huang wrote:

And I still don't see any reasons why should tools stick to rc1 if
we are moving forward to rc2 or final 1.0.0. In other words, why do
we want validation for multiple spec versions simultaneously before
1.0.0?

I'm ok just generating/validating the latest released spec before
1.0, although it would be nice if we had a plan for supporting
multiple versions after 1.0 (to meet out backwards-compatibility
mandate [1]). I don't see a need to support random spec commits:

$ ./oci-runtime-tool generate | head -n2
{
"ociVersion": "1.0.0-rc1-dev",

when changes to the config semantics between the current RCs are small
(as they have been so far). There have been larger changes to the
spec's Go types between RCs, but I'd rather address those with the PR
that bumps us to the next release. Chasing runtime-spec's Go types
between releases is energy I'd rather not spend ;).

With rc3 still not tagged, runtimes would be foolish to claim support
for 1.0.0-rc2-dev (maybe some breaking change lands before rc3). And
I see no point in claiming support for the old rc1-dev when rc2 has
already been cut. I expect the only reason we have a runtime that
passes validation is that runC ignores ociVersion entirely
(opencontainers/runc#521) and assumes your feeding it the sort of
config it can handle (bleeding-edge with some redundant mounts #24).

So I'd rather support multiple versions cleanly (e.g. via #98 with
something equally flexible to drive the runtime validation). And if
we don't want to do that, I'd rather support the latest tagged spec
release. I see no upside to supporting -dev spec commits.

[1]: https://www.opencontainers.org/about/governance §7.g

@mrunalp
Copy link
Contributor

mrunalp commented Nov 7, 2016

LGTM

Approved with PullApprove

@mrunalp mrunalp merged commit 414d0e8 into opencontainers:master Nov 7, 2016
@Mashimiao Mashimiao deleted the generate-specified-typo-fix branch December 29, 2016 07:25
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.

5 participants