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

Enhance status check label for promotions #3263

Merged
merged 2 commits into from
Nov 22, 2022

Conversation

michelangelomo
Copy link

This PR introduces a change for labels applied to status checks related to promotions, actually a generic one is used.

@michelangelomo
Copy link
Author

hi @bradrydzewski, we would like to use this change asap, could you drop a review? 🙏

@tphoney
Copy link

tphoney commented Sep 13, 2022

I had a look at the change, and this seems to be additive so it shouldnt affect any existing functionality. I will defer to Brad, as i have not looked at the lifecycle of statuses in depth

@aleinside
Copy link

hi @tphoney, any feedback from @bradrydzewski? 🙏

@michelangelomo
Copy link
Author

@tphoney @bradrydzewski bump

@bradrydzewski
Copy link

bradrydzewski commented Nov 9, 2022

The deploy value is freeform and may contain special characters, spaces, etc. @tphoney do we know that all providers (github, gitlab, gitea, bitbucket, stash, azure) support freeform text and no character restrictions? If yes, then we are good to merge. If no, I would recommend taking the slug value (using the existing slug library that we already import).

import (
	"fmt"

	"github.com/drone/drone/core"
	"github.com/drone/go-scm/scm"

+	github.com/gosimple/slug
)
	case core.EventPromote:
-		return fmt.Sprintf("%s/promote/%s", name, deployTo)
+		return fmt.Sprintf("%s/promote/%s", name, slug.Make(deployTo))

@michelangelomo
Copy link
Author

The deploy value is freeform and may contain special characters, spaces, etc. @tphoney do we know that all providers (github, gitlab, gitea, bitbucket, stash, azure) support freeform text and no character restrictions? If yes, then we are good to merge. If no, I would recommend taking the slug value (using the existing slug library that we already import).

hey @bradrydzewski, I think your suggestion is reasonable even if may there are no special character, better safe than sorry 😄

@SharpEdgeMarshall
Copy link

Any update on this?

Copy link
Contributor

@d1wilko d1wilko left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for the change @michelangelomo - apologies for the delay

@d1wilko d1wilko merged commit fa73a85 into harness:master Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants