CFE-1007: CI configuration for openshift/coredns-ocp-dnsnameresolver #46743
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@arkadeepsen: This pull request references CFE-1007 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci
bot
added
the
approved
|
/assign @alebedev87 |
|
/uncc |
| base_images: | ||
| base: | ||
| name: "4.16" | ||
| namespace: ocp | ||
| tag: base |
There was a problem hiding this comment.
Do we need the base_images? There is no images section in this CI job. That is, no images is built by the job. Normally we use base_images in images section to make some substitutions in Dockerfile's FROM instruction.
| promotion: | ||
| name: "4.16" | ||
| namespace: ocp |
There was a problem hiding this comment.
No image is built by this CI job, so there is nothing to promote.
| releases: | ||
| initial: | ||
| integration: | ||
| name: "4.16" | ||
| namespace: ocp | ||
| latest: | ||
| integration: | ||
| include_built_images: true | ||
| name: "4.16" | ||
| namespace: ocp |
There was a problem hiding this comment.
This job doesn't need OCP for any of the test steps.
| requests: | ||
| cpu: 100m | ||
| memory: 200Mi | ||
| tests: |
There was a problem hiding this comment.
I think that it may make sense to add verify step to run go fmt and other commands.
| lgtm: | ||
| - repos: | ||
| - openshift/coredns-ocp-dnsnameresolver | ||
| review_acts_as_lgtm: true |
There was a problem hiding this comment.
That is better to be set to false because this allows people not mentioned in the OWNERS file to give approvals to PRs.
| - commandHelpLink: "" | ||
| repos: | ||
| - openshift/coredns-ocp-dnsnameresolver | ||
| require_self_approval: false |
There was a problem hiding this comment.
This setting reduces the number of people required to make the PR pass from 2 to 1 if the creator is from the OWNERS list. I think it's better to have an explicit approval and LGTM labels from 2 different reviews, especially for a code shared between teams.
| @@ -0,0 +1,13 @@ | |||
| approve: | |||
There was a problem hiding this comment.
Another setting which can be useful is ignore_review_state: true (ref). It disallows the GitHub review to impact the approvals of the PR.
openshift-ci
bot
added
the
do-not-merge/invalid-owners-file
|
LGTM, just waiting for the owner file's problem to be fixed. |
openshift-merge-robot
added
the
needs-rebase
openshift-ci
bot
removed
the
approved
arkadeepsen
force-pushed
the
coredns-ocp-dnsnameresolver
branch
from
24b5a0f
to
87c43e7
Compare
openshift-merge-robot
removed
the
needs-rebase
openshift-ci
bot
added
approved
|
/lgtm Holding for @gcs278 to have a look. |
openshift-ci
bot
added
do-not-merge/hold
|
/pj-rehearse |
|
@alebedev87 I ran pj-rehearse but the tests are failing because the repo is empty and the Makefile is not there. I was referring to this issue earlier. |
|
@arkadeepsen: sorry I misunderstood. Yes, you are right, the initial PR is not merged yet, the rehearsal won't work. Thanks for following up on this one! |
| @@ -0,0 +1,14 @@ | |||
| tide: | |||
| queries: | |||
| - labels: | |||
There was a problem hiding this comment.
What's the reason these other labels are not included?
labels:
- approved
- backport-risk-assessed
- cherry-pick-approved
- jira/valid-bug
- jira/valid-reference
- lgtm
There was a problem hiding this comment.
Thanks for pointing that out. I have now used the prow config for tide from cluster-dns-operator directory. The reference for each field can be found here: https://docs.prow.k8s.io/docs/components/core/tide/config/#queries
There was a problem hiding this comment.
I noticed the simple labeling strategy too but I didn't ask to align it with the other NetEdge repos for the reason of simplifying the initial development process. However here I may be talking about 3 labels more: docs/qe/px approved.
There was a problem hiding this comment.
I see. I don't mind if you want add the label requirements now, or later. But I think we can apply all the labels we need manually if you want to shortcut the labels during development.
There was a problem hiding this comment.
| lgtm: | ||
| - repos: | ||
| - openshift/coredns-ocp-dnsnameresolver | ||
| plugins: |
There was a problem hiding this comment.
Is there a reason we are not including the restricted_labels? I don't totally understand this repo, but does our normal NE label process apply to it?
label:
restricted_labels:
openshift/coredns:
- allowed_users:
- candita
- Miciah
label: backport-risk-assessed
- allowed_users:
- lihongan
- quarterpin
- melvinjoseph86
- ShudiLi
assign_on:
- label: backport-risk-assessed
label: cherry-pick-approved
There was a problem hiding this comment.
I have updated the config for labels as well for the NE process.
openshift-ci-robot
added
the
rehearsals-ack
| - community-4.6 | ||
| - community-4.7 | ||
| - openshift-4.10 | ||
| - openshift-4.11 | ||
| - openshift-4.12 | ||
| - openshift-4.13 | ||
| - openshift-4.14 | ||
| - openshift-4.2 | ||
| - openshift-4.3 | ||
| - openshift-4.4 | ||
| - openshift-4.5 | ||
| - openshift-4.6 | ||
| - openshift-4.7 | ||
| - openshift-4.8 | ||
| - openshift-4.9 | ||
| - release-4.0 | ||
| - release-4.10 | ||
| - release-4.11 | ||
| - release-4.12 | ||
| - release-4.13 | ||
| - release-4.14 | ||
| - release-4.2 | ||
| - release-4.3 | ||
| - release-4.4 | ||
| - release-4.5 | ||
| - release-4.6 | ||
| - release-4.7 | ||
| - release-4.8 | ||
| - release-4.9 |
There was a problem hiding this comment.
Why all these branches for a new repository?
There was a problem hiding this comment.
These branches are used in all the prowconfig files in different directories. I checked most of them. I used the prowconfig file from the cluster-dns-operator as that will adhere to the labeling used by the NE team.
There was a problem hiding this comment.
Well, for the cluster-dns-operator they make sense as the operator existed from the very first version of OCP 4. We don't have these branches for the new plugin. This may be misleading keeping the old release branches like if the plugin was delivered for those releases too or like it has those branches. For the new releases though, I suppose that ART automation will add new branches.
There was a problem hiding this comment.
Btw this config is for the backports as well as the config right after it (openshift-4.15). It feels a little premature to add the CI config for the backports while we don't have the initial release yet. For the moment, we cannot say which releases we may need to backport something to. I think I'd prefer to keep the CI config to the necessary minimum - main branch only, and add new things when we need them. WDYT?
There was a problem hiding this comment.
Makes sense. We can always update the file as and when required. I will make the appropriate changes.
| - feature-es6x | ||
| - feature-prom-connector | ||
| - main | ||
| - master |
There was a problem hiding this comment.
How all these branches relate to the dnsnamesresolver plugin?
There was a problem hiding this comment.
This is a common format for prowconfig across repos belonging to openshift. Most of the repos don't have the branches. The prowconfig files are updated through automation during branch cut and release. I am guessing that this format may have something to do with the automation.
| - openshift/coredns-ocp-dnsnameresolver | ||
| - includedBranches: | ||
| - main | ||
| - master |
There was a problem hiding this comment.
Why master branch? The plugin's default branch is main.
There was a problem hiding this comment.
Same as #46743 (comment)
| label: backport-risk-assessed | ||
| - allowed_users: | ||
| - lihongan | ||
| - quarterpin |
There was a problem hiding this comment.
quarterpin you can remove this one, as he is no more with the organization
There was a problem hiding this comment.
Thanks @melvinjoseph86 . Will update it. Should there be anybody else who needs to be added here?
There was a problem hiding this comment.
We are only three now...
openshift-ci-robot
removed
the
rehearsals-ack
arkadeepsen
force-pushed
the
coredns-ocp-dnsnameresolver
branch
from
0f788bd
to
aeb3366
Compare
| - main | ||
| labels: | ||
| - approved | ||
| - jira/valid-reference |
There was a problem hiding this comment.
Shouldn't we have jira/valid-bug label too here? This part seems to be for the bugs.
|
/lgtm |
arkadeepsen
force-pushed
the
coredns-ocp-dnsnameresolver
branch
from
f606b90
to
811ad9f
Compare
|
@arkadeepsen: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/lgtm |
|
@gcs278 can you please again take a look? If everything is fine then we can merge this PR. |
|
Reapplying /pj-rehearse ack |
openshift-ci-robot
added
the
rehearsals-ack
|
Yea I'll admit I don't totally understand all of the mechanisms/plugins/config here, but look at the comparisons to our existing config seem to mostly align. This repo also not in production yet, so I think we still have time to tweak things if we get them wrong. |
openshift-ci
bot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alebedev87, arkadeepsen, gcs278 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
1 similar comment
|
/retest |
|
@arkadeepsen: Updated the following 2 configmaps:
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
…penshift#46743) * CI configuration for openshift/coredns-ocp-dnsnameresolver * Add OWNERS file for openshift/coredns-ocp-dnsnameresolver * Updated CI configuration and OWNERS file * Executed "make jobs" and updated correct username in OWNERS file * Update pluginconfig and prowconfig for correct label and tide configurations respectively * Fixed the repo name * Update pluginconfig and prowconfig * Added jira/valid-bug to prowconfig
…penshift#46743) * CI configuration for openshift/coredns-ocp-dnsnameresolver * Add OWNERS file for openshift/coredns-ocp-dnsnameresolver * Updated CI configuration and OWNERS file * Executed "make jobs" and updated correct username in OWNERS file * Update pluginconfig and prowconfig for correct label and tide configurations respectively * Fixed the repo name * Update pluginconfig and prowconfig * Added jira/valid-bug to prowconfig
…penshift#46743) * CI configuration for openshift/coredns-ocp-dnsnameresolver * Add OWNERS file for openshift/coredns-ocp-dnsnameresolver * Updated CI configuration and OWNERS file * Executed "make jobs" and updated correct username in OWNERS file * Update pluginconfig and prowconfig for correct label and tide configurations respectively * Fixed the repo name * Update pluginconfig and prowconfig * Added jira/valid-bug to prowconfig
This PR adds the CI configuration for the new repo openshift/coredns-ocp-dnsnameresolver.