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

Updates to slsutil.banner #53499

Merged
merged 7 commits into from
Aug 13, 2019
Merged

Updates to slsutil.banner #53499

merged 7 commits into from
Aug 13, 2019

Conversation

amendlik
Copy link
Contributor

What does this PR do?

Address a number of issues with slsutil.banner

  1. Typo in the default banner text
  2. Issue when the comment character was padded with a space
  3. More examples in the documentation
  4. Unhandled exception when the width was too small

What issues does this PR fix or reference?

None

Tests written?

Yes

Commits signed with GPG?

Yes

@amendlik amendlik changed the title Banner Updates to slsutil.banner Jun 15, 2019
@Ch3LL Ch3LL requested a review from cmcmarrow June 18, 2019 14:10
@amendlik
Copy link
Contributor Author

@cmcmarrow We'll want to put this on the neon branch, now that that exists.

@amendlik
Copy link
Contributor Author

amendlik commented Jul 1, 2019

@Ch3LL, @cmcmarrow Any update on this PR?

@Ch3LL Ch3LL added bugfix-bckport will be be back-ported to an older release branch by creating a PR against that branch ZRELEASED - Neon retired label labels Jul 2, 2019
@Ch3LL
Copy link
Contributor

Ch3LL commented Jul 2, 2019

@amendlik i went ahead and added it to be backported to neon once this is merged.

bump @cmcmarrow waiting on your review here.

@amendlik
Copy link
Contributor Author

amendlik commented Jul 8, 2019

@Ch3LL, can you contact @cmcmarrow and let him know we're waiting on him? I had another PR that sat for 6 weeks and later found out the assignee had his GitHub notifications turned off. Maybe that is what's going on here.

@Ch3LL Ch3LL requested a review from a team as a code owner July 30, 2019 15:05
cmcmarrow
cmcmarrow suggested changes Jul 30, 2019
View reviewed changes
salt/modules/slsutil.py Outdated

########################################################################
# #
# Copyright 2019 ACME Corp #
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change 'ACME Corp' to Salt for this example?

Copy link
Contributor

Choose a reason for hiding this comment

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

Its because ACME Corp is a thing. https://www.acmecorp.com/

@cmcmarrow
Copy link
Contributor

@amendlik I just have one request for change. Other than that it looks good. Thanks for the test!

@Ch3LL
Copy link
Contributor

Ch3LL commented Aug 6, 2019

ping @cmcmarrow this is ready for your re-review

@amendlik
Copy link
Contributor Author

@cmcmarrow @Ch3LL What is going on at SaltStack that PR's now take 2 months to be merged? It used to be a week at the most. I don't mean to sound critical, but the level of support for contributors has dropped drastically over the last year, and SaltStack has done nothing to manage expectations.

@Ch3LL
Copy link
Contributor

Ch3LL commented Aug 13, 2019

@amendlik we have been slowing down merges as we attempt to stabilize our test suite. There's a bit more explanation here: https://groups.google.com/forum/#!topic/salt-announce/5RJjtDYszx8

We want to lower the gap between merge and release time, but unfortunately that requires us to slow down on merges as we ensure our test suite is stabilized. I'll ping @cmcmarrow offline because it looks like we might be able to get this one in. Let me know if I can clarify anything else.

@dwoz dwoz merged commit c9c3a4b into saltstack:develop Aug 13, 2019
@amendlik amendlik mentioned this pull request Apr 21, 2020
Merged
@s0undt3ch s0undt3ch added the has master-port port to master has been created label Apr 21, 2020
@s0undt3ch
Copy link
Collaborator

Master port #56761

@amendlik amendlik deleted the banner branch October 16, 2020 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmcmarrow cmcmarrow cmcmarrow requested changes

Assignees
No one assigned
Labels
bugfix-bckport will be be back-ported to an older release branch by creating a PR against that branch code-jam has master-port port to master has been created Reviewers-Assigned ZRELEASED - Neon retired label
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@amendlik @Ch3LL @cmcmarrow @s0undt3ch @dwoz @sagetherage