-
Notifications
You must be signed in to change notification settings - Fork 238
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
Update pipeline-service #1725
Update pipeline-service #1725
Conversation
Roming22
commented
Apr 25, 2023
- OSP-1.10
- PaC deployed through OSP-1.10
- allow for watcher logging level customization via gitops
I would think we would want to remove tekton-chains at https://github.com/openshift-pipelines/pipeline-service/tree/main/operator/gitops/argocd/pipeline-service/tekton-chains as well @Roming22 and validate that removal via pipeline service CI before updating staging Admittedly though, I'm a little unclear about how we override openshift-pipelines over at https://github.com/openshift-pipelines/pipeline-service/tree/main/operator/gitops/argocd/pipeline-service/openshift-pipelines so as to install or not install tekton-chains of course, maybe I'm wrong here .... can you elaborate ? thanks |
@gabemontero I'm not sure I understand why removing tekton-chains is a pre-requiste to upgrading to OSP-1.10. We certainly want to remove it, sooner than later, but it's going to take some time before it's ready, and if possible I'd like to validate that OSP-1.10 is running smoothly as soon as possible to avoid last minute surprises before the freeze. |
I thought PaC and tekton-chains were the same in that we had to install them from upstream to get required levels, but at 1.10 both the PaC and tekton-chains installed by openshift-pipelines were at sufficient levels What motivated the removal of PaC via openshift-pipelines/pipeline-service#620 to be included, bu not chains ... I would think we would want to be consistent. Please clarify when you get the chance, but I won't block merge, assuming e2e's pass (they did not on the first go) /lgtm |
and the e2e failed with a gitops hiccup wrt pipeline service:
at first quick glance can't say for certain if it is related to the bump ... /test appstudio-e2e-tests |
@Roming22 can you update the commit message to add "using tls for rds connections" as result of this update ? |
/retest |
@Roming22 - unfortunately the latest run https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/redhat-appstudio_infra-deployments/1725/pull-ci-redhat-appstudio-infra-deployments-main-appstudio-e2e-tests/1650981219980021760 failed in the same way as what I reported in #1725 (comment) I think that points to gitops / kustomize is not happy with something in pipeline-service between By rough count I see 21 or 22 commits between those 2 commits, so if I am correct there is a large set to choose from, unless the error message wrt My best guess based on that clue is that it is somehow related to openshift-pipelines/pipeline-service@a07a5a0 from @Mo3m3n based on a comment in that file, but that is certainly guess that is more wild than educated, especially since that merged before |
The external secrets for Pipelines as Code probably need to be updated. I think the PR as is will break staging because PaC will be deployed in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
@Mo3m3n @adambkaplan Comments processed. SHA updated, and there's no references to the
|
/lgtm |
pr needs a rebase @Roming22 do you want to do it, or I can hit the "update with rebase" button if you like |
/lgtm |
@Roming22 I have just found that you also need to update this line https://github.com/redhat-appstudio/infra-deployments/blob/main/hack/secret-creator/create-plnsvc-secrets.sh#L27 to match this change done here in pipeline-service openshift-pipelines/pipeline-service@b5f1dc2#diff-f394b1c9744cc285b6ac80384e967bd73210dd72a905e1ad7581b8ecab4e3773R201 Basically when switching to the helm based postgres deployment the name of the db service changed too (for dev mode). That's why I suggested in my comment to also add to the commit message that we are enabling tls with this update and using Helm for postgres deployment |
@Mo3m3n - we minimally need #1737 to merge in order to pick up @mmorhun 's fix for with namespace PaC is installed in. It has not merged as I type this comment. For what you noted with https://github.com/redhat-appstudio/infra-deployments/tree/main/components/authentication and https://github.com/redhat-appstudio/infra-deployments/tree/main/components/sprayproxy yes that is part of what @Michkov noted in #1725 (comment) I would say @Roming22 each of those need to be addressed as part of this PR Once all that happens, we can see about rebase/retest and try to merge |
@Roming22 just pulled ^^ into this PR :-)
|
@Michkov I believe I've updated all references to pipelines-as-code. I tried to make sure not to impact production by patching where necessary. |
the last CI failure needs #1579 we just (possibly re-)triaged that mvp-demo failure in https://redhat-internal.slack.com/archives/C02FANRBZQD/p1682697746956539 |
hopefully konflux-ci/e2e-tests#488 fixes this |
/lgtm |
more new unrelated flakes being discussed in slack (I've asked QE to open blocking bugs and put skips in these failing tests until they are sorted out). /retest |
/retest |
I see the same By comparison, #1741 is showing green with the older version of PaC. Might be time to try out this locally, work with @psturc and @flacatus to understand the details of this test, see where the breakdown is. |
Issue in build-service:
|
New changes are detected. LGTM label has been removed. |
* OSP-1.10 * PaC deployed through OSP-1.10 * Allow for watcher logging level customization via gitops * Using TLS for RDS connections Signed-off-by: Romain Arnaud <rarnaud@redhat.com>
|
So still the same error in the last e2e run. @Roming22 - so in my local repro of this, I killed the build-service pod, and when it restarted, it no longer got the I wonder if this is a startup order sort of thing, where the build-service initializes its client before the openshift-pipelines operator has "fully installed things", where as previously, PaC was getting created by pipeline-service directly Here are the current sync-waves I see for the two:
|
looks like we officially have multiple problems |
@Roming22: The following test 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. |
Closed in favor of #1769 We'll be taking baby steps:
|