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

Add PR locking configuration at the digger.yml level #1578

Merged
merged 5 commits into from
Jun 21, 2024

Conversation

evanstachowiak
Copy link
Contributor

@evanstachowiak evanstachowiak commented Jun 14, 2024

Following the conversation here https://diggertalk.slack.com/archives/C01DVJRK6CX/p1718200181031169.

Adding the ability to enable/disable PR locking in GitHub with a digger.yml config setting.

Addresses this issue: #1577

Copy link
Contributor

@motatoes motatoes left a comment

Choose a reason for hiding this comment

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

Thanks for this. Overall looks good and I'm happy about the naming.

We also need to override locking to be noopLock if this flag is set for backendless mode here:

diggerConfig, diggerConfigYaml, dependencyGraph, err := digger_config.LoadDiggerConfig("./", true, nil)

if err != nil {
utils.InitCommentReporter(ghService, prNumber, fmt.Sprintf(":x: Failed perform lock action on project: %v %v", project.Name, err))
return fmt.Errorf("failed to perform lock action on project: %v, %v", project.Name, err)
if dg_configuration.PrLocks{
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to use the config object here:

Suggested change
if dg_configuration.PrLocks{
if config.PrLocks{

if err != nil {
utils.InitCommentReporter(ghService, prNumber, fmt.Sprintf(":x: Failed perform lock action on project: %v %v", project.Name, err))
return fmt.Errorf("failed to perform lock action on project: %v, %v", project.Name, err)
if dg_configuration.PrLocks{
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to do the same if statement in the handleIssueComment webhook handler in the same file

@evanstachowiak
Copy link
Contributor Author

I added support for Enable on the struct level, I couldn't see a better way to do it for the cli. In this case I could probably also remove these if statements around the for loops, but it would also save some processing to leave them in.

@motatoes
Copy link
Contributor

Hey @evanstachowiak this looks great, thanks! I would rather not have a flag on the PrLock interface. We have a NoopLock interface implemented here:

type NoOpLock struct {
which will ignore any lock operations

so if you had something like

if config.Prlocks {
    log.Printf("info: using NoopLock)
    lock = NoopLock{}
}

here it will do the trick - since that path is outside of the orchestrator path.

@motatoes
Copy link
Contributor

@evanstachowiak actually thinking about it again, your changes will work well, the enable flag works. Future refactoring can be done but this is good enough for now!

@motatoes motatoes merged commit cb1df67 into diggerhq:develop Jun 21, 2024
3 of 6 checks passed
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.

2 participants