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.

Sign up for GitHub

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

Jump to bottom

pull in openshift/library updates, including jenkins agent streams #88

Merged
merged 2 commits into from
Feb 1, 2019
Merged

pull in openshift/library updates, including jenkins agent streams #88

merged 2 commits into from
Feb 1, 2019

Conversation

gabemontero
Copy link
Contributor

No description provided.

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 30, 2019
@gabemontero gabemontero added the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2019
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@gabemontero
Copy link
Contributor Author

image-eco was terraform / aws network hiccups on startup

@gabemontero gabemontero removed the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2019
@gabemontero
Copy link
Contributor Author

Hmm ... the new agent imagestrem / image ref stuff needs some investigating / unit testing ... isn't turn key as is

First, the envs we expected are not there on startup:

time="2019-01-30T21:01:59Z" level=warning msg="The environment variable IMAGE_AGENT_MAVEN was not set and we cannot update the jenkins-agent-maven:v4.0 image references"
time="2019-01-30T21:01:59Z" level=warning msg="The environment variable IMAGE_AGENT_NODEJS was not set and we cannot update the jenkins-agent-nodejs:v4.0 image references"
time="2019-01-30T21:01:59Z" level=warning msg="The environment variable IMAGE_JENKINS was not set and we cannot update the jenkins:2 image references"

Also only seeing jenkins-agent- in the in progress reason field.

@gabemontero gabemontero mentioned this pull request Jan 31, 2019
Merged
@gabemontero
Copy link
Contributor Author

/retest

1 similar comment
@gabemontero
Copy link
Contributor Author

/retest

@gabemontero
Copy link
Contributor Author

I'm seeing the env's set at least locally now ... perhaps a question of moving parts under the api.ci and various release plumbing

Will start testing locally

Have not seen e2e-aws-operator test have its cluster successfully started up yet ... will retest in a bit

@gabemontero
Copy link
Contributor Author

found the current issue ... the import in progress reason field maintenance could not deal with jenkins-agent-nodejs vs. nodejs

coding fix now

@gabemontero
Copy link
Contributor Author

Also determined that the

time="2019-01-30T21:01:59Z" level=warning msg="The environment variable IMAGE_AGENT_MAVEN was not set and we cannot update the jenkins-agent-maven:v4.0 image references"
time="2019-01-30T21:01:59Z" level=warning msg="The environment variable IMAGE_AGENT_NODEJS was not set and we cannot update the jenkins-agent-nodejs:v4.0 image references"
time="2019-01-30T21:01:59Z" level=warning msg="The environment variable IMAGE_JENKINS was not set and we cannot update the jenkins:2 image references"

messages in the e2e output are red herrings ... they come from the fact the e2e tests call the operator file function as part of confirming the correct samples content is in fact in place ... those env's are not set on the test e2e process

@gabemontero gabemontero mentioned this pull request Jan 31, 2019
Merged
@gabemontero
Copy link
Contributor Author

commit to fix #88 (comment) pushed

@bparees ptal

bparees
bparees reviewed Jan 31, 2019
View reviewed changes
pkg/stub/jenkins.go
@@ -15,6 +15,7 @@ func tagInPayload(tag, env string, stream *imagev1.ImageStream) *imagev1.ImageSt
}
for _, tagSpec := range stream.Spec.Tags {
if tagSpec.Name == tag {
logrus.Printf("updating image ref for tag %s in stream %s with image %s", tag, stream.Name, imageRef)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be debug level?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually in the end I consciously chose non debug level ... I'd liked having this information in there

these messages are infrequent compared to the other messaging

still want me to make it debug?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

falls under my statement from a few weeks ago that at some point we need to audit our default log messages, but for now it's ok.

@bparees
Copy link
Contributor

bparees commented Jan 31, 2019

one nit, otherwise lgtm

@bparees
Copy link
Contributor

bparees commented Jan 31, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 31, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, gabemontero

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:
  • OWNERS [bparees,gabemontero]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gabemontero
Copy link
Contributor Author

ok e2e-aws-operator passed ... good news

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 058a364 into openshift:master Feb 1, 2019
@gabemontero gabemontero deleted the bump-lib branch February 1, 2019 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bparees bparees bparees left review comments

@coreydaley coreydaley Awaiting requested review from coreydaley

@legionus legionus Awaiting requested review from legionus

Assignees

@bparees bparees

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@gabemontero @openshift-bot @bparees @openshift-ci-robot @openshift-merge-robot