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

[ci] use shorter workflow and job names #75333

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

umarcor
Copy link
Contributor

@umarcor umarcor commented Mar 25, 2023

Current CI job titles are too long for the visualisation in GitHub, both at the bottom of PRs and in the Actions sidebar. This PR makes the names shorter by removing the content in parens.

@umarcor umarcor requested a review from a team as a code owner March 25, 2023 23:17
@YuriSizov
Copy link
Contributor

While this makes the names shorter, this is useful information. And removing the trailing part does not improve which part of the name is visible.

@umarcor
Copy link
Contributor Author

umarcor commented Mar 26, 2023

@YuriSizov long names hide execution time of the jobs at the bottom of PRs.

Currently:

image

In this PR:

image

Furthermore, at least the target argument feels redundant because all the jobs with target editor are named Editor and all the ones with target template_release are named Template.

If the information is useful (particularly in the four jobs with longer names), I believe there are better solutions to show it. Currently they can only be read by clicking on the 'Details' and going to the job run view (e.g. https://github.com/godotengine/godot/actions/runs/4522307823/jobs/7964678551?pr=75325). It's not readable in the sidebar, nor in the graph. Therefore, we might have a step that prints those arguments, right after 'Set up job'. That would show the info in the same page (just a couple of rows below), and it would fix the issues.

Moreover, we might use the GITHUB_STEP_SUMMARY to have that info shown at the bottom of the summary, next to the list of artifacts. What do you prefer?

@umarcor
Copy link
Contributor Author

umarcor commented Mar 26, 2023

I updated this PR to print the "INFO" as the first step of the jobs. I did not include the "target" field, so the jobs that had that argument only don't have an INFO field. Click on the Details of the checks in this PR to see the output. For instance: https://github.com/godotengine/godot/actions/runs/4526609969/jobs/7971901828?pr=75333.

Moreover, branch https://github.com/umarcor/godot/tree/umarcor/ci/info is one commit ahead of this one. That prints the content to GITHUB_STEP_SUMMARY. As a result, apart from the info being shown as a step in each job, it's shown in the summary of jobs. See, for instance: https://github.com/umarcor/godot/actions/runs/4526572433/jobs/7971836148 and https://github.com/umarcor/godot/actions/runs/4526572433. Note that the summary is shown after the job is finished, not right after the step is executed.

@YuriSizov
Copy link
Contributor

long names hide execution time of the jobs at the bottom of PRs.

Is this the only reason for the changes? Why is the execution time of the CI important? It's not a stable value, it fluctuates throughout the day and depending on the Actions agents availability. So it doesn't provide any useful information to contributors. It's useful mostly to some engine maintainers to investigate if we have a congestion or other issues. But we don't seem to have an issue with the current way things are.

I appreciate the effort, but this sounds like a non-problem, and thus there is no need to look for a solution.

@umarcor
Copy link
Contributor Author

umarcor commented Mar 26, 2023

Execution of CI is important for contributors who are waiting for CI to finish...

Yet the reason for the changes is semantic. The name or title should contain the name or title, not metadata. Doing so breaks 3 of the 4 places where the name is shown in the GitHub web frontend. I find it difficult to understand why that's a no problem since it feels as an obvious symptom that something is not being used as it should. Using features as intended prevents future unexpected issues.

Could you please explain why the current implementation in this PR does not satisfy the needs of those who need to see that metadata?

@umarcor umarcor changed the title [ci] use shorter job names [ci] use shorter workflow and job names Mar 26, 2023
@YuriSizov
Copy link
Contributor

YuriSizov commented Mar 26, 2023

Execution of CI is important for contributors who are waiting for CI to finish...

Whether a CI run has finished or not is displayed with a status icon. The time being visible doesn't help with waiting whatsoever.

Using features as intended prevents future unexpected issues.

If there are any actual issues, we can work on them and discuss a solution. Making these changes just because some of the UI doesn't look neat and clean is not a good rationale.

Could you please explain why the current implementation in this PR does not satisfy the needs of those who need to see that metadata?

The point of having this data in a very visible place is for it to be easily visible. Both during runs and during configuration/reading the config. Prints are hidden in the execution steps. We already print all configuration flags during the actual compilation, so adding an extra info print step doesn't actually add anything useful or make it easier to find.

Again, contributions should come from actual problems first, and I don't see an actual problem being solved here.

The name or title should contain the name or title, not metadata.

The name should contain what we want it to contain. You are indeed creating a semantical problem with an arbitrary limitation in this statement.

@umarcor
Copy link
Contributor Author

umarcor commented Mar 27, 2023

The time being visible doesn't help with waiting whatsoever.

It's the status, the time, plus whatever GitHub decides to dynamically put after the job name since the PR is created/pushed until it's merged.

Execution time can be important when a job failed because it lets the contributor know if something is preventing the job from starting, or it did start but it failed after several minutes.

If there are any actual issues, we can work on them and discuss a solution. Making these changes just because some of the UI doesn't look neat and clean is not a good rationale.

GitHub users might want to use the features provided by GitHub. I personally like reading the status information at the bottom of the PRs when I'm contributing to a project.

Is your judgement to decide whether breaking UI is irrelevant? I would please ask to have this discussion opened to other maintainers. It seems to be an opinion that having redundant text breaking UI is worth.

The point of having this data in a very visible place is for it to be easily visible. Both during runs and during configuration/reading the config. Prints are hidden in the execution steps. We already print all configuration flags during the actual compilation, so adding an extra info print step doesn't actually add anything useful or make it easier to find.

Visibility and readability during runs:

current,

image

in this PR,

image

and the flags print during compilation (which requires scrolling and expanding one step),

image

Visibility and readability when reading the config:

current,

image

in this PR,

image

Again, contributions should come from actual problems first, and I don't see an actual problem being solved here.

According to your previous statements, you deem it negligible, but you do see the problem, which is UI being broken and info being hidden in the PR checks view.

The name should contain what we want it to contain. You are indeed creating a semantical problem with an arbitrary limitation in this statement.

The arbitrary limitation I'm trying to create is "do not break the UI unnecessarily since users might want to use it". I believe it's rather sensible.

@YuriSizov
Copy link
Contributor

I'm sorry, I don't think the build time is relevant. It's not a useful piece of information, and I would ask you to trust me on that, because I look at dozens of Godot PRs daily (including waiting on them to finish so I can merge them). Maybe it's something that can be referred to in other projects, but at our scale, build time is not something a regular contributor can use in a meaningful way (and relying on it can be misleading).

So that's a non-problem to me that the UI "breaks". And thus I don't think that moving the important text to the build logs is justified, even if you can make an argument that your proposed logs are slightly better than existing logs.

@umarcor
Copy link
Contributor Author

umarcor commented Mar 27, 2023

@YuriSizov, I beg not to accept the conversation shifting towards ad verecundiam. You did several statements related to technical issues which were incorrect and I felt I should clarify which are the facts. Now, it's obviously a matter of opinion, and you are in position to make it prevail over others.
Hence, feel free to close this as "wontfix" based on your criteria, or just wait to see if other people want to participate in the discussion.

Alternatively, you might review the content of this PR and focus on the changes that are useful, instead of pointing the ones you don't like.

@YuriSizov
Copy link
Contributor

@umarcor Please don't devalue an opinion of a maintainer and a stakeholder who has more experience in the area by referring to a debate technique. I'm discussing your improvements here not because I have nothing better to do and just need to establish my authority, but because I want to help you understand the due process and the fact that you don't present a compelling case for an issue to be fixed in the first place. I can't review your PR because you propose changes to address a problem that is not a problem to anyone to my knowledge.

This is not a matter of opinions, and this is not a matter of authority. So instead of closing it as "wontfix" I'd prefer to help you understand what it is that I'm saying and why I'm spending time discussing this rather minor change with you. If you'd rather maintainers didn't discuss the merits of your PRs with you, then I don't know what to suggest to you.

@yedpodtrzitko
Copy link
Contributor

yedpodtrzitko commented Mar 28, 2023

just wait to see if other people want to participate in the discussion.

let me chip in from a position of very casual contributor. @umarcor I think I am in the same shoes as you are - very eager to help, and where else to help than in the domain you understand the best. Such contributions are definitely welcome, that's the way how open source projects can flourish. Unfortunately for me Godot is primarily the game engine and C which is not my domain. So I can keep digging through the other parts which I understand (ci/buildsystems), keep discovering non-issues there, which then I can "fix". Sometime I hit, sometime I miss.

In this particular PR, what issue does this solve? All boils down to "build time is not shown" (?)
I agree with Yuri here - this information is not really useful in the big picture of the project. You mentioned "execution of CI is important for contributors who are waiting for CI to finish." Never heard anyone anywhere mentioning this. Do you have any datapoints supporting this claim please? Even if this would be a real issue, this information is still not useful - the build times are unpredictable so seeing the execution time doesnt help anything. And even then if this information would be really essential to have, then the issue should be directed to Github and the UI of Actions rather than the project here.

So I would say if it's not broken dont fix it. There are easily hundreds of similar non-issues in the project, but I think instead of bikeshedding about them back and forth we should let the maintainers/devs focus on the important things.

@Riteo
Copy link
Contributor

Riteo commented Mar 28, 2023

Nah, IMO this change is pretty unneeded and Yuri's arguments make sense.

As a general rule of thumb, stuff must not be done if it doesn't solve a real problem. I can think of very few ""exceptions"" but, if this has only been done to show the CI completion time, I can see very few uses out of that value, especially as PRs are extremely atomic here and thus, even if times would be "stable", they would hardly change between PRs.

I'm not completely against the idea of this change, if we had a different workflow there might be a good reason for this, I'm just against the problems it's trying to solve, which IMO don't exist.

Also, the title is still handier than a log entry IMO.

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

4 participants