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

refactor/players_online_count #39

Merged
merged 3 commits into from
Nov 9, 2024

Conversation

changchiyou
Copy link
Contributor

Background Summary

Currently !online command directly display the value of global varaible playersOnline:

FactoriGOChatBot/main.go

Lines 448 to 450 in 143ba61

case "!online":
messagesToDiscord <- strconv.Itoa(playersOnline) + " players online"
break

, which is plused/minused by parseAndFormatMessage:

FactoriGOChatBot/main.go

Lines 150 to 165 in 143ba61

case "JOIN":
playersOnline += 1
if !config.sendJoinLeave {
return ""
}
var re = regexp.MustCompile(`(?m)] (\w*)`)
match := re.FindStringSubmatch(message)
return fmt.Sprintf(":green_circle: | `%s` joined the game!", match[1])
case "LEAVE":
playersOnline -= 1
if !config.sendJoinLeave {
return ""
}
var re = regexp.MustCompile(`(?m)] (\w*)`)
match := re.FindStringSubmatch(message)
return fmt.Sprintf(":red_circle: | `%s` left the game!", match[1])

and updated by updatePlayerCount:

FactoriGOChatBot/main.go

Lines 401 to 417 in 143ba61

func updatePlayerCount(rconClient *rcon.Client) {
msg, err := rconClient.Execute("/players online count")
if err != nil {
log.WithFields(logrus.Fields{"err": err}).Error("Could not get player count from Factorio")
return
}
playersOnline, err = strconv.Atoi(strings.Split(strings.Split(msg, "(")[1], ")")[0])
if err != nil {
log.WithFields(logrus.Fields{"err": err}).Panic("Could not parse player count from Factorio")
return
}
if playersOnline > 0 {
updateDiscordStatus(discordgo.ActivityTypeWatching, "the factory grow")
} else {
updateDiscordStatus(discordgo.ActivityTypeWatching, "the world burn")
}
}

I noticed that the parseAndFormatMessage function's mechanism for increasing or decreasing the online user(player) count in JOIN/LEAVE scenarios fails to calculate correctly due to repeated logins after abnormal disconnections, such as unexpected logouts.

Discord console.log
image image

Therefore, I've refactored the functions related to playersOnline. Now, whenever !online is entered, FactoriGOChatBot will re-execute /players online count on the server to ensure the latest player count is retrieved. Additionally, parseAndFormatMessage will no longer affect playersOnline to ensure the stability of the online player count functionality..

…unt`

remove unstable counting method in `parseAndFormatMessage`'s JOIN/LEAVE cases
@Mattie112
Copy link
Owner

Sounds good, ignore the failing pipeline I need to look into that why PRs made by others can't trigger the pipeline. I will test it this weekend!

@changchiyou
Copy link
Contributor Author

changchiyou commented Nov 9, 2024

@Mattie112 Using secrets in GitHub Actions - Using secrets in a workflow

With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

I recommend modifying your workflow to follow the Git Flow methodology . At a minimum, you should create a new branch named dev or develop to handle pull requests (PRs) from forked repositories. Additionally, consider splitting the current GitHub Action into two parts. Once a PR passed the checks about CI and is approved by the repo owner(you) in the dev branch, merge it into the main branch then trigger the checks about CD using Github secrets.

@Mattie112
Copy link
Owner

Yeah, it was my intention to have a quick way for everyone to test changes/PR that is why I thought to just have everything pushed to docker. It didn't occur to me that that would not work :)

But yeah I think you are right and that an additional branch is in order and merge from that into main.

main.go Outdated Show resolved Hide resolved
@Mattie112 Mattie112 merged commit 6824784 into Mattie112:main Nov 9, 2024
1 check failed
@Mattie112
Copy link
Owner

Thanks for the PR, merged :)

Will look into the branch structure / pipelines!

@changchiyou changchiyou deleted the refactor/players_online_count branch November 10, 2024 09:44
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