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

Log link to build details, don't print JSON [CLOUDDST-2404] #17

Merged

Conversation

neithere
Copy link

The pubtools-iib CLI commands used to dump the build details JSON
into stdout. Pub would then call the commands, capture their stdout
and stderr output and add it to its own logs.

Such verbose output is unnecessary because the JSON is available
on the IIB server via HTTP API.

This PR removes the JSON from the logs and replaces it with the URL.

@neithere
Copy link
Author

@midnightercz has approved, could others PTAL too? @lipoja @emilyzheng @querti @chandwanitulsi @yashvardhannanavati
Thanks!

Copy link
Member

@rohanpm rohanpm left a comment

Choose a reason for hiding this comment

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

Is there a need for a corresponding Pub change to prevent CI breaking there?

pubtools/iib/iib_ops.py Outdated Show resolved Hide resolved
pubtools/iib/iib_ops.py Outdated Show resolved Hide resolved
pubtools/iib/iib_ops.py Outdated Show resolved Hide resolved
tests/test_iib_ops.py Show resolved Hide resolved
@neithere
Copy link
Author

Is there a need for a corresponding Pub change to prevent CI breaking there?

None I'm aware of. AFAIK, Pub's integration tests mock the entry point, so the pubtools-iib's output seems to be covered only by its own unit tests. Is there anything else?

@rohanpm
Copy link
Member

rohanpm commented Nov 11, 2020

AFAIK, Pub's integration tests mock the entry point, so the pubtools-iib's output seems to be covered only by its own unit tests. Is there anything else?

I don't know the answer, but CLOUDDST-2404 says one of the motivations is that, with the old approach, the Pub task log "contains full output of iib task which is also not desired as it can be changed easily and breaks pub CI". And I remember something like that happening, but have not looked into whether this specific change is covering that incident exactly.

Something here wouldn't make sense if CLOUDDST-2404 on the one hand says "let's change the behavior because current behavior can break pub's CI" and on the other hand we're saying "this change will have no impact on Pub's CI", so I would have guessed that this indeed does affect the behavior of some test in Pub.

Regardless, I'm clicking Approve on this since it's not a problem with this PR as such. But if it hasn't been looked into, it probably should be before releasing this.

@chandwanitulsi chandwanitulsi merged commit f413ed9 into release-engineering:master Nov 12, 2020
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.

None yet

4 participants