-
-
Notifications
You must be signed in to change notification settings - Fork 65
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
Test app/depot configurable with secrets and add DLC support (doesn't use appId + 1 for first depot) #26
Conversation
I've created a new DLC and Steam Build Account and confirmed tests are passing with all that: I'm happy to offer the use of that DLC and build account for this repo if desired, as discussed in #18 |
Also confirmed that standard |
Hmm I set up all the secrets needed to get the test passing on this PR, but it didn't work. It looks like the input is being ignored for some reason. Here's the input in this PR:
Here's the input in the source commit on my forked repo:
Same commit, same secrets 🤔 I guess GitHub Actions is being a little funny. Might sort itself out after merging? |
I guess it kinda makes sense though. If secrets were used in arbitrary PRs, then it'd be easy for strangers to Indeed, this is intentional, just found this in settings:
|
Considering this, I've added a new commit which disables running the test workflow on PRs since it can't possibly pass ever, I think 🤔 |
The primary branch ( Actually the PR workflow passes if one of us core maintainers pushes a change, as the secrets are allowed to be used. |
@@ -23,7 +22,8 @@ jobs: | |||
configVdf: ${{ secrets.STEAM_CONFIG_VDF}} | |||
ssfnFileName: ${{ secrets.STEAM_SSFN_FILE_NAME }} | |||
ssfnFileContents: ${{ secrets.STEAM_SSFN_FILE_CONTENTS }} | |||
appId: 1234560 | |||
appId: ${{ secrets.TEST_APP_ID }} | |||
firstDepotIdOverride: ${{ secrets.TEST_FIRST_DEPOT_ID_OVERRIDE }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would extend the public API of this action.
Is there no way to just use the recommended approach from steam themselves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with this. What recommended approach are you referring to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With public API I simply mean what you configure the action with:
with:
parameter: value
In general, as part of GameCI Actions philosophy we try to limit the amount of parameters that the end-user would have to understand to a minimum. We always try to deduce information internally inside the action where possible.
Perhaps it's not possible to do that in this case, but I don't have sufficient context to judge this. This is why I'm asking as a question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry I understand the aversion to modifying the public API - my question was more about this part:
Is there no way to just use the recommended approach from steam themselves?
I'm not sure what recommended approach from Steam you're referring to here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of context, what I've learned in this exercise is that a certain configuration of DLC in Steamworks results in a first depot whose ID is not appId + 1
, but rather is exactly equal to appId
, so the existing functionality wouldn't have worked for that 😢
Even thought this DLC was just made for testing, it's highlighted a situation that real users can face if they're using Game.CI to build/deploy some DLC.
So I added this as a backwards-compatible option for handling this DLC case, and possibly other cases too that we're still not aware of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we would not mix multiple concerns in the same PR, but I understand it might be needed to get the workflow running.
LGTM
Changes
appId + 1
pattern$TEST_APP_ID
and$TEST_FIRST_DEPOT_ID_OVERRIDE
secrets so the test app/depot can be configured without needing to change codeChecklist