switch to unified github context string: suse/cloud #3055

jdsn · 2018-12-21T14:34:26Z

The previous context string "suse/mkcloud" was specific
to mkcloud. So it would be misleading to let jobs testing
other deployment tools report a status using this context
string.

On the other hand adding new context strings and requiring
them as mandatory for a merge would mean that on most PRs
some checks will never be run, because not all PRs run with
all deployment tools.

So this new context string

must have a success status which is mandatory for a merge
will be used for the status of the default test (PR build)
of this PR.
Hint: this is the "standard" job, that is defined in the
github-pr yaml in:
https://github.com/SUSE-Cloud/automation/blob/master/scripts/github_pr/github_pr_cloud.yaml#L5
The "standard" is not added as suffix to the context, while
all other build names (keys on the same level in the yaml) are
appended as suffix to the base context.
is the base string for all additional (optional and mandatory)
contexts that are required (this is already the case).
e.g.
old: suse/mkcloud/stable
new: suse/cloud/stable

The previous context string "suse/mkcloud" was specific to mkcloud. So it would be misleading to let jobs testing other deployment tools report a status using this context string. On the other hand adding new context strings and requiring them as mandatory for a merge would mean that on most PRs some checks will never be run, because not all PRs run with all deployment tools So this new context string - must have a success status which is mandatory for a merge - will be used for the status of the default test (PR build) of this PR. Hint: this is the "standard" job, that is defined in the github-pr yaml in: https://github.com/SUSE-Cloud/automation/blob/master/scripts/github_pr/github_pr_cloud.yaml#L5 The "standard" is not added as suffix to the context, while all other build names (keys on the same level in the yaml) are appended as suffix to the base context. - is the base string for all additional (optional and mandatory) contexts that are required (this is already the case). e.g. old: suse/mkcloud/stable new: suse/cloud/stable

jdsn · 2018-12-21T14:35:31Z

Please note
This PR is a preparation for the switch of the context string during the planned downtime.
Do NOT merge it yet.

aspiers

Kudos on the detailed commit message! I really appreciate the effort here. Nevertheless I still have some nitpicks, sorry ;-)

The previous context string "suse/mkcloud" was specific to mkcloud.

Here you are assuming that reviewers all know exactly what a context string is. Given this morning's discussion, I think this is not at all a safe assumption. Better to include a quick explanation which links to the relevant section in the API docs.

So it would be misleading to let jobs testing other
deployment tools report a status using this context string.

Worth citing Ardana / CLM here for clarity.

On the other hand adding new context strings

Does this imply "adding new context strings whilst keeping suse/mkcloud"? If so, better to be explicit.

and requiring them as mandatory for a merge would mean that on most PRs some checks will never be run, because not all PRs run with all deployment tools

This is a little hard for me to understand. If you are talking about PRs only on this automation repo, then it's worth explicitly stating that the problem is that it has to deal with separate PRs for Crowbar and Ardana, and so Crowbar checks should only be required on Crowbar-related PRs, and similarly for Ardana. I find that references to "some checks" and "all deployment tools" are a bit vague.

Also, what about PRs on other repos, such as the Crowbar ones? Are they out of scope for this PR?

So this new context string

must have a success status which is mandatory for a merge

I don't follow this - how can a context string have a success status? It's just a string.

will be used for the status of the default test (PR build)
of this PR.

What is the default test? Where is that defined? When can the default be overridden, and when it is, how does that impact the context string?

Hint: this is the "standard" job, that is defined in the
github-pr yaml in:
/scripts/github_pr/github_pr_cloud.yaml@master#L5
The "standard" is not added as suffix to the context, while
all other build names (keys on the same level in the yaml) are
appended as suffix to the base context.

Why this special case, instead of always having a suffix?

is the base string for all additional (optional and mandatory)
contexts that are required (this is already the case).

Optional contexts that are required? Isn't that a contradiction, or am I missing something?

e.g.
old: suse/mkcloud/stable
new: suse/cloud/stable

This example helps. More examples would help even more :-)

BTW I know this is a lot of questions. Based on this morning's discussion I think I probably know answers to some of these, but I'm still not 100% sure and anyway the commit message is for the benefit of the whole team, not just me :-)

aspiers · 2018-12-21T17:35:03Z

jenkins/ci.suse.de/cloud-crowbar-testbuild-pr.yaml

@@ -84,23 +84,24 @@
        github_pr_sha=${github_opts[1]}
        github_pr_branch=${github_opts[2]}
        ghpr_paras="--org ${crowbar_org} --repo ${crowbar_repo} --sha ${github_pr_sha}"
+        gh_context="suse/cloud"


Shouldn't this be gh_context_prefix? It's not the whole context.

Yes, I agree, gh_context_prefix is a more meaningful name for the variable.

aspiers · 2018-12-21T17:35:25Z

scripts/jenkins/mkcloud/openstack-mkcloud.builder.sh

-    github_context=suse/mkcloud
-    if [[ $github_pr_context ]] ; then
-      github_context=$github_context/$github_pr_context
+    github_context=suse/cloud


Same here - github_context_prefix?

aspiers · 2018-12-21T17:35:51Z

scripts/jenkins/mkcloud/openstack-mkcloud.builder.sh

-      github_context=$github_context/$github_pr_context
+    github_context=suse/cloud
+    if [[ $github_pr_context_suffix ]] ; then
+      github_context=$github_context/$github_pr_context_suffix


github_context=$github_context_prefix/$github_pr_context_suffix

dirkmueller · 2019-01-02T17:58:36Z

have we considered changing this to suse/cloud/mkcloud/stable instead of suse/cloud/stable ? stable is a very generic term and it was much more specific before.

jdsn · 2019-01-06T20:41:06Z

@dirkmueller

have we considered changing this to suse/cloud/mkcloud/stable instead of suse/cloud/stable ?

No. The agreement was to switch to suse/cloud as base context (context string for the default CI job). All additional jobs will append their suffix to this. So, an "ha" build will report to: suse/cloud/ha.
And the stable build (which is the build against the currently 'stable' product) will report to suse/cloud/stable.

stable is a very generic term and it was much more specific before.

We already have the stable suffix for the stable builds as described. This will not change. We can do the change, sure. But please only lets have one big change.

I just wanted to do the switch this night. In case you prefer to add other changes, I am open to postpone this action by a week or two (maybe this is a topic for a breakout session in two weeks?).

dirkmueller · 2019-01-07T07:46:26Z

@dirkmueller

have we considered changing this to suse/cloud/mkcloud/stable instead of suse/cloud/stable ?

No. The agreement was to switch to suse/cloud as base context (context string for the default CI job).

I'm not disagreeing with the change of the base context (context prefix). with that change the specific that this is a "mkcloud" job is lost however, thats why I asked whether we want to add mkcloud/ as a prefix to the suffix (hah, good luck with parsing that !)

e.g.

-context_prefix=suse/mkcloud
+context_prefix=suse/cloud
-context_suffix=stable
+context_suffix=mkcloud/stable

(this is pseudo code)

aspiers · 2019-01-07T13:49:14Z

I agree with Dirk. We should not lose the mkcloud substring, and his proposal seems like the obvious way to achieve that whilst still moving to a generic suse/cloud base context.

ConsoleCatzirl

If we have one job that reports back with the base context prefix, and all other jobs report with a suffix appended, how do we ensure that the additional jobs are not treated as second-class citizens? For example, we have both openstack-mkcloud and openstack-ardana and we should be treating both equally. We want them both to be mandatory, and reporting a skipped test should look similar for both jobs.

Perhaps we should deprecate the base context for the time being, and give all jobs a context suffix. So we could have suse/cloud/mkcloud and suse/cloud/ardana both as mandatory, and reserve suse/cloud for when we have converged on a single lifecycle manager (or a single job that tests both manager paths).

ConsoleCatzirl · 2019-01-18T09:09:49Z

jenkins/ci.suse.de/cloud-crowbar-testbuild-pr.yaml

@@ -84,23 +84,24 @@
        github_pr_sha=${github_opts[1]}
        github_pr_branch=${github_opts[2]}
        ghpr_paras="--org ${crowbar_org} --repo ${crowbar_repo} --sha ${github_pr_sha}"
+        gh_context="suse/cloud"


Yes, I agree, gh_context_prefix is a more meaningful name for the variable.

aspiers · 2019-01-21T09:36:51Z

I agree with @jesusaurus's point about treating them equally. I think @dirkmueller's proposal should tick all the boxes and keep everyone happy?

ConsoleCatzirl · 2019-01-25T19:22:58Z

@aspiers yeah, I think we're all happy with @dirkmueller's proposal

jdsn added the do not merge yet label Dec 21, 2018

jdsn requested review from aspiers, bmwiedemann, stefannica and rsalevsky December 21, 2018 14:34

aspiers suggested changes Dec 21, 2018

View reviewed changes

ConsoleCatzirl reviewed Jan 18, 2019

View reviewed changes

stefannica removed request for stefannica and rsalevsky March 11, 2024 08:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

switch to unified github context string: suse/cloud #3055

switch to unified github context string: suse/cloud #3055

jdsn commented Dec 21, 2018

jdsn commented Dec 21, 2018 •

edited

Loading

aspiers left a comment

aspiers Dec 21, 2018

ConsoleCatzirl Jan 18, 2019

aspiers Dec 21, 2018

aspiers Dec 21, 2018

dirkmueller commented Jan 2, 2019

jdsn commented Jan 6, 2019 •

edited

Loading

dirkmueller commented Jan 7, 2019 •

edited

Loading

aspiers commented Jan 7, 2019

ConsoleCatzirl left a comment

ConsoleCatzirl Jan 18, 2019

aspiers commented Jan 21, 2019

ConsoleCatzirl commented Jan 25, 2019

switch to unified github context string: suse/cloud #3055

Are you sure you want to change the base?

switch to unified github context string: suse/cloud #3055

Conversation

jdsn commented Dec 21, 2018

jdsn commented Dec 21, 2018 • edited Loading

aspiers left a comment

Choose a reason for hiding this comment

aspiers Dec 21, 2018

Choose a reason for hiding this comment

ConsoleCatzirl Jan 18, 2019

Choose a reason for hiding this comment

aspiers Dec 21, 2018

Choose a reason for hiding this comment

aspiers Dec 21, 2018

Choose a reason for hiding this comment

dirkmueller commented Jan 2, 2019

jdsn commented Jan 6, 2019 • edited Loading

dirkmueller commented Jan 7, 2019 • edited Loading

aspiers commented Jan 7, 2019

ConsoleCatzirl left a comment

Choose a reason for hiding this comment

ConsoleCatzirl Jan 18, 2019

Choose a reason for hiding this comment

aspiers commented Jan 21, 2019

ConsoleCatzirl commented Jan 25, 2019

jdsn commented Dec 21, 2018 •

edited

Loading

jdsn commented Jan 6, 2019 •

edited

Loading

dirkmueller commented Jan 7, 2019 •

edited

Loading