Skip to content

octopus: cephadm ls broken for SUSE downstream alertmanager container#39802

Merged
tchaikov merged 1 commit intoceph:octopusfrom
p-se:wip-pse-cephadm-SUSE-alertmanager-octopus
Apr 27, 2021
Merged

octopus: cephadm ls broken for SUSE downstream alertmanager container#39802
tchaikov merged 1 commit intoceph:octopusfrom
p-se:wip-pse-cephadm-SUSE-alertmanager-octopus

Conversation

@p-se
Copy link
Contributor

@p-se p-se commented Mar 3, 2021

Binary in downstream container is named prometheus-alertmanager instead of alertmanager.

Checklist

  • References tracker ticket
  • Updates documentation if necessary
  • Includes tests for new functionality or reproducer for bug
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@p-se p-se requested a review from a team as a code owner March 3, 2021 09:33
@p-se p-se changed the title cephadm: cephadm ls broken for SUSE downstream alertmanager container octopus: cephadm ls broken for SUSE downstream alertmanager container Mar 3, 2021
@p-se p-se force-pushed the wip-pse-cephadm-SUSE-alertmanager-octopus branch from d28734e to f3e70ec Compare March 3, 2021 09:38
@sebastian-philipp sebastian-philipp added this to the octopus milestone Mar 3, 2021
sebastian-philipp
sebastian-philipp suggested changes Mar 3, 2021
View reviewed changes
Copy link
Contributor

@sebastian-philipp sebastian-philipp left a comment

Choose a reason for hiding this comment

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

when doing the cherry-pick and you encounter conflicts, please remove the # characters from the commit message that highlight conflicts.

See 514fc60 for an example

@smithfarm
Copy link
Contributor

smithfarm commented Mar 9, 2021

@p-se This appears to be a cherry-pick of #39722 - correct?

Did you combine all three commits from #39722 into a single commit?

Commits from master should be cherry-picked individually. If you only need to cherry-pick one commit from master, then could you please explain in the PR description why the other two commits were omitted?

@p-se
Copy link
Contributor Author

p-se commented Mar 10, 2021

@p-se This appears to be a cherry-pick of #39722 - correct?

Did you combine all three commits from #39722 into a single commit?

Commits from master should be cherry-picked individually. If you only need to cherry-pick one commit from master, then could you please explain in the PR description why the other two commits were omitted?

@smithfarm I combined two commits from master into one that should've been one commit in the first place, but I failed to squash them in the PR to master. Which commits that are is in the commit message.

@p-se p-se requested a review from sebastian-philipp March 10, 2021 08:34
@p-se
cephadm: cephadm ls broken for SUSE downstream alertmanager container
d69db78 
Fixes: https://tracker.ceph.com/issues/49506

Signed-off-by: Patrick Seidensal <pseidensal@suse.com>
(cherry picked from commit 8f0dae0)
(cherry picked from commit 3c222a8)

Conflicts:
    src/cephadm/cephadm
    src/cephadm/tests/test_cephadm.py
@p-se p-se force-pushed the wip-pse-cephadm-SUSE-alertmanager-octopus branch from f3e70ec to d69db78 Compare March 10, 2021 08:53
@smithfarm
Copy link
Contributor

smithfarm commented Mar 10, 2021

@p-se What about the third commit from the master PR? I'm assuming that was omitted here for a reason, but I don't see that noted anywhere?

@p-se
Copy link
Contributor Author

p-se commented Mar 11, 2021 •
edited
Loading

@p-se What about the third commit from the master PR? I'm assuming that was omitted here for a reason, but I don't see that noted anywhere?

@smithfarm The third commit does not resolve the issue that is supposed to be fixed. It does, strictly speaking not belong to the PR (neither master nor this one). Hence, I didn't think it would be necessary to explain why it wasn't cherry picked. The third commit also doesn't have that Fixes line. It was just some minor cleanup I did along the way.

Do you think it should be either part of this backport or be mentioned in the commit message, why it wasn't cherry picked?

@smithfarm
Copy link
Contributor

smithfarm commented Mar 11, 2021

Do you think it should be either part of this backport or be mentioned in the commit message, why it wasn't cherry picked?

@p-se It's enough to mention that somewhere in this PR, which you just did ;-) Part of reviewing a backport is verifying that all commits from the master PR were cherry-picked. So it helps reviewers if you provide an explanation of any differences up front.

@tchaikov
Copy link
Contributor

tchaikov commented Apr 14, 2021

i think most of cephadm test failures are caused by https://tracker.ceph.com/issues/49293

@p-se p-se requested a review from mgfritch April 20, 2021 10:01
@idryomov idryomov changed the base branch from octopus to octopus-saved April 20, 2021 18:12
@idryomov idryomov changed the base branch from octopus-saved to octopus April 20, 2021 18:12
@tchaikov
Copy link
Contributor

tchaikov commented Apr 25, 2021

jenkins test api

@tchaikov tchaikov merged commit 5407cd4 into ceph:octopus Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liewegas liewegas Awaiting requested review from liewegas

@votdev votdev Awaiting requested review from votdev

@pcuzner pcuzner Awaiting requested review from pcuzner

@mgfritch mgfritch Awaiting requested review from mgfritch

1 more reviewer

@sebastian-philipp sebastian-philipp sebastian-philipp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cephadm octopus-batch-1

Projects

None yet

Milestone

octopus

Development

Successfully merging this pull request may close these issues.

4 participants

@p-se @smithfarm @tchaikov @sebastian-philipp