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

fix dockerRepoVersion #99

Merged
merged 1 commit into from
Dec 23, 2023

Conversation

VeyronSakai
Copy link
Contributor

@VeyronSakai VeyronSakai commented Dec 17, 2023

Changes

Checklist

  • Read the contribution guide and accept the code of conduct
  • Readme (updated or not needed)
  • Tests (added, updated or not needed)

@VeyronSakai VeyronSakai marked this pull request as ready for review December 17, 2023 03:53
@GabLeRoux
Copy link
Member

Activation got reworked with recent release (v4) of builder and test-runner actions. I'm not sure unity-request-activation-file is still required, but it's probably worth updating this anyway.

Copy link
Member

@AndrewKahr AndrewKahr left a comment

Choose a reason for hiding this comment

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

I'm not sure we want to merge this. This repo needs to be updated to print a message directing users to follow updated activations instructions and error out as this cannot be used to get license files anymore and then have the repo put into public archive. Updating the images means it will be more likely to inadvertently work and confuse new users following old documentation.

@webbertakken
Copy link
Member

I could agree with either of your statements.

On the one hand, it's not guaranteed that the old way of activating is completely obsolete for everyone. In which case @GabLeRoux is probably right that we should maybe naively upgrade it.

At the same time it might make sense to put a disclaimer that there's a likeliness one wants to use the updated mechanism instead.

Perhaps a hybrid solution between the two? WDYT @AndrewKahr?

@AndrewKahr
Copy link
Member

@webbertakken So there's a few reasons I'm recommending scorched earth for this action but leaving the other activate/return license actions alone:

  1. This action is used primarily by people who are getting setup with GameCI. This is the optimal time to intercept them and point them to the correct resources. This is also a manual action so we won't be breaking anyone's pipelines overnight.
  2. The biggest issue with this activation flow is Unity has gotten rid of the ability to use the file this action generates on this site. Thus our ability to maintain this action doesn't actually help people use the flow. While there still is a messy workaround, I suspect it will disappear fairly soon rendering it useless. This means people instead get caught on not being able to activate their license on Unity's site which will offer no guidance rather than failing out in the action and providing optimal instructions on how to proceed. I suspect if it's just a warning or info log level, people aren't going to read it and just download the license file anyway.
  3. The only people that are supposed to use the ALF activation method for GameCI are personal license users, but Unity has locked it to be paid users only. However, those users should be using the serial method anyway. Thus the only usage now would be for people running offline which inherently shouldn't work since it's a CI system which needs network connectivity to run in the first place. Hence I believe the actual audience for this action is effectively nobody.

The other activate/return license steps are automated and do more than our normal activation flow so I wouldn't want to impact those at all, however this repo effectively serves no role other than potential confusion to new users. Hence I recommend we simply empty the action out, throw a core.error with instructions to follow to the activation docs, update the readme about the changes, then mark the repo public archive.

@webbertakken
Copy link
Member

This action is used primarily by people who are getting setup with GameCI.

Perhaps primarily, perhaps not even that anymore. But there are people that don't use GameCI workflows, just this action and the return license. So we can not just drop it completely because the mainstream use case has changed.

As there literally are thousands of projects using our actions, I want to make sure that we don't cut off people that were still using it. If you're sure we're not cutting off anyone then I'm fine, but otherwise I prefer a more hybrid approach. Perhaps even adopt the same activation as we do in builder.

I suspect if it's just a warning or info log level, people aren't going to read it and just download the license file anyway.

I agree with everything you said before, but I don't agree that people don't read warnings, especially when just setting up a workflow. We should use best practices and use warning and error levels appropriately.

@AndrewKahr
Copy link
Member

Perhaps even adopt the same activation as we do in builder.

I think there might be some confusion over the function of this repo. This repo is only to generate the (now defunct) alf file for the web tool Unity provides. But since Unity is removing the ability to use that web tool, that is why I don't think we should continue allowing this action to function as it will lead to more user confusion. Even for people who use old versions of unity-builder or their own tooling, it doesn't help them to generate the alf since the Unity side of the equation has now been removed which makes the alfs unusable. The activate/return actions will be left alone as those continue to provide value (and actually function).

However, what I think we can do to be safe is merge this PR and tag it as v2.1, then merge a following PR that empties the action except for an error message with guidance on activation and tag it as v2.2 such that it becomes the latest for v2. That way new people following old docs will get v2.2 with guidance and if people have a need they can pin to v2.1 and open an issue so we can understand what that need is. Then when it's clear that nobody has any issues without the action (maybe wait 6 months), we can update the readme indicating the action is indeed unused and set the repo to public archive.

Thoughts on this strategy @webbertakken?

@webbertakken
Copy link
Member

Sounds good. Let's do it!

@AndrewKahr AndrewKahr merged commit 0ae5da4 into game-ci:main Dec 23, 2023
5 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.

4 participants