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

Rename Makefile targets #6104

Merged
merged 31 commits into from
Jun 26, 2024
Merged

Rename Makefile targets #6104

merged 31 commits into from
Jun 26, 2024

Conversation

sneridagh
Copy link
Member

@sneridagh sneridagh commented Jun 15, 2024


📚 Documentation preview 📚: https://volto--6104.org.readthedocs.build/

Copy link

netlify bot commented Jun 15, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit 6b799d9
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/667a9e27a247f500082f0c72

Copy link
Member

@ichim-david ichim-david left a comment

Choose a reason for hiding this comment

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

@sneridagh it's nice that we are getting some sort of pattern normalization.
Personally I would get rid of the ci- so we have
acceptance-backend-start
acceptance-frontend-start
acceptance-plone5-backend-start
acceptance-multilingual-frontend-start
to me it sounds better when reading than plone5-acceptance-backend-start
but i'm ok with what the majority decides.
Curious of what others prefer order wise so no review or request changes,
just a comment.

@sneridagh
Copy link
Member Author

sneridagh commented Jun 15, 2024

@ichim-david we decided this back during the Mexico sprint, @stevepiercy made a proposal for cookieplone. It's already implemented in there. I had the same question that you are saying, the "recipient" before, or after the "thing to be actioned". I also was a bit confused about the ci- but @stevepiercy convinced us.

@stevepiercy What do you think? Also, did we write it down somewhere? I think it was in the cookieplone-templates repo, right?

@ichim-david
Copy link
Member

@sneridagh @stevepiercy Looking at the docs
https://6.docs.plone.org/volto/contributing/acceptance-tests.html#how-to-run-acceptance-tests-locally-during-development
I do wish there was a set of commands that we want users to use for testing:
make acceptance-backend
make acceptance-frontend
make acceptance-test

easy to explain, you need to run the commands that will make the necessary actions to start the backend frontend and test infrastructure.
Then if we have a ton of commands that we use for our CI, ok make it as complicated as you want but for the end user do this before committing to having a simple scheme to remember without any extra bells and whisles.
I wasn't part of Mexico sprint and I don't know what was decided, again I will defer to the decision taken by the majority however whatever is decided I hope it's easy to remember and is properly documented.

@stevepiercy
Copy link
Collaborator

See plone/cookiecutter-volto#14 (comment) for the full spec on naming.

@ichim-david ichim-david self-requested a review June 16, 2024 05:31
Copy link
Member

@ichim-david ichim-david left a comment

Choose a reason for hiding this comment

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

We need:

  1. Docs update to mention the new commands for end users
  2. Breaking notice and information in the upgrade guide that we have different names for doing acceptance testing

Question:
What are we going todo about 16.x.x and 17.x.x branches?
Are we going to keep the old naming or change it there as well?

Looking at the comment
plone/cookiecutter-volto#14 (comment)
I see:
acceptance-test Start Cypress in interactive mode
Given that this command mentions start as well I would keep it then tethered to the backend and frontend naming thus: acceptance-test-start.
If this is too late to change well I least I gave my opinion :)

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

I started a review, then aborted. I would like to collect all the Makefile commands as they currently exist and document them in a table, then provide their new target and description. It will serve as a universal change log for all things that use the commands.

https://docs.google.com/spreadsheets/d/1C_kBeXrx5nRLYaLI8qNXhBOBD9j7Ty0r_DxJNDz7Cww/edit?gid=0#gid=0

All can view, request edit access as needed.

@sneridagh
Copy link
Member Author

We need:

  1. Docs update to mention the new commands for end users
  2. Breaking notice and information in the upgrade guide that we have different names for doing acceptance testing

I added an upgrade guide entry to make the change more noticeable. Since it's a Volto only change, it only applies to the core development itself, it's not breaking anything external.

Question: What are we going todo about 16.x.x and 17.x.x branches? Are we going to keep the old naming or change it there as well?

That's a good question, and tbh, after making the effort of unifying it in 18, I have no more stamina left for doing this in 17 or 16... If someone is up to do it, fine for me. Also, remember that when 18 is out, 17 will be legacy. At the end this is for improving core development, and to unify things along with the new setup in Cookieplone.

@stevepiercy
Copy link
Collaborator

Who maintains cmscom.jp? It's timing out in the readme linkcheck. @tisto @terapyon.

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

I still need to do the inner Makefile, but let's get the one at the root finalized.

@stevepiercy
Copy link
Collaborator

@tisto conversely having dozens of start-* and stop-* makes it difficult to find the thing that you want to stop or start. Personally I find this much easier to use because there is clear differentiation between the things I want to use. I also don't accidentally start or stop the thing that is one item above or below my target.

@stevepiercy
Copy link
Collaborator

@sneridagh @stevepiercy can we get the doc build to work? I wanted to look at the table but I get a 404 when visiting https://volto--6104.org.readthedocs.build/

Done in ca219b3.

Link to New naming convention for Makefile commands.

@ichim-david
Copy link
Member

ichim-david commented Jun 18, 2024

@stevepiercy I find it a bit strange that you made changes to ensure that different words are split by - and
yet in the table I see
docs-linkcheckbroken
copyreleasenotestodocs
now they just seem out of place and it would be a shame not to have everything clean 99% of it is already :)

@tisto
Copy link
Member

tisto commented Jun 18, 2024

conversely having dozens of start-* and stop-* makes it difficult to find the thing that you want to stop or start.

@stevepiercy I honestly don't get this argument. The number of start-* commands is something like 4-5, and the number of overall commands is 20-30 (just guessing here). How can it be more difficult to choose from 4-5 items than finding the right item in 20-30 items?

The argument that you can accidentally do something is true for any command line argument. I also honestly don't see how the decision for or against the current status quo could make any difference in that regard.

@@ -30,19 +30,19 @@ All sessions should start from the `packages/volto` directory.
1. In the first session, start the backend server.

```shell
make start-test-acceptance-server
make acceptance-backend-start
Copy link
Member

Choose a reason for hiding this comment

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

@stevepiercy I think a note mentioning that they were changed from volto 18 would bring
more awareness. we do have notes about commands in other sections and given that this
is something that is required for a good pull request it's better to give the user a heads-up.
In the same route I feel that the upgrade guide regarding make commands should mention
what area might be affected by this change.
Something along the lines:
Most of these commands were used internally by Volto core for its development, however,
one change that will affect most users is the change regarding the performing of acceptance tests.
Have a look at the acceptance docs section for the new commands used.
@sneridagh what do you think about this?

Another thing that we could do is to have for this version some convenience declarations

start-test-acceptance-server:
echo "start-test-acceptance-server has been renamed, use:
make acceptance-frontend-start"
It's only 3-4 commands and we give the user the ability to quickly do acceptance testing
without having to go and study everything that has changed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a bad example, because the command was documented incorrectly in the first place (it used to apply for CI), and was not a simple rename. Otherwise, yes, warnings of changes may be appropriate. For this one case, I would not bother.

Victor and I've already invested too many hours in getting makefile commands unified, and we are both weary from it. It's tedious. I am "meh" on adding echo because developers can do make help to find the command they want to use, or consult the Upgrade Guide. I think it would be helpful, but I just don't have the energy to champion this enhancement.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ichim-david

Regarding the first thing, we could add the deprecation notice. However, I tend to follow Steve suggestions on this regard. At the end, (unfortunately) the core developers are not counted by the thousands.

Regarding the last suggestion. I'm with Steve in this one. This is the PR from hell of the season, if someone wants to go the extra mile, please be my guest. IMHO we have better things to take care of. The users of this are core developers, we will manage.

@tisto
Copy link
Member

tisto commented Jun 18, 2024

@stevepiercy @sneridagh @ericof I was about to let go this discussion when I started to think about what bothers me so much about this change. Now I remember. Let me explain:

We are defining an API here for programmers who do not necessarily know the details about the underlying infrastructure (devs that know that their way will find it in any case).

The reason why I am so opposed to this change is that as a "regular user" I encountered the pattern that we are proposing when using process managers. I think this is a good example for the devs we are aiming to build this "user interface" for.

When using a process manager like Supervisord or PM2 I don't care about any details of the underlying implementation. For me, this is just a tool that I want to use.

I checked both Supervisord and PM2:

They both use the format " <start|stop|restart> "

I don't recall if Supervisord changed their format at some point or if it was another tool (Pyramid, Python WSGI Server?). However, I still recall how frustrated I was every single time I worked with the format " <start|restart|stop>" because it felt so unnatural to me. And this wasn't something that just went away over time.

The reason for this is that I can easily recall "start|stop|restart" but I can never recall the services that I might have (and we have lots of services in Volto these days that might be easy to remember for core devs but not for me or any other seasonal dev). Autocomplete will only help me if I can type "start-" to see all services that I have. It won't help if I type "make", then I will see all commands which is overwhelming.

Anyways. Process managers are process managers and Volto is Volto. I might be wrong here. Plone is a do-ocracy and I did not contribute anything to the decision-making process or the code change so far. I just wanted to share my gut feeling that this is a wrong move IMHO.

If we really care about our devs and to get this right we could use the current proposal and find some guinea pig developers and watch them use the new structure. However, I don't have any devs at hand that do not know the old structure. Therefore they will just complain about any change and this is not a valid user test.

@sneridagh
Copy link
Member Author

@tisto
I see your point, it's valid as any other. At the end is to decide on one, and go along with it.
Adopt this change is a bit painful (as all changes), but once you get the grasp, it's as any other, you have to learn it once. I know because I didn't like it at the beginning, but at the end you get used to it quickly, and it's not that bad. At the end is naming...

@stevepiercy I think that what happens is that the list of available commands is so big, that it's daunting. We should write a list of the most useful commands.

Seasonal devs will only have to know this:

make install:  ## Set up development environment
make start: ## Starts Volto, allowing reloading of the add-on during development
make backend-docker-start: ## Starts a Docker-based backend for development

That's it. It is unlikely that seasonal devs runs locally Cypress, but if that is the case they are documented since some time ago, and this is the summary:

make acceptance-frontend-dev-start: ## Start acceptance frontend in development mode
make acceptance-backend-start: ## Start backend acceptance server
make acceptance-test: ## Start Cypress in interactive mode

FTR the previous commands were:

make start-test-acceptance-frontend-dev
make start-test-acceptance-server-start
make test-acceptance

Then, we have the different fixtures for each we have a corresponding commands as above:

make multilingual-acceptance-frontend-prod-start: ## Start acceptance frontend in production mode for multilingual tests
make multilingual-acceptance-backend-start: ## Start backend acceptance server for multilingual tests
make multilingual-acceptance-test: ## Start Cypress in interactive mode for multilingual tests

I don't see that the difference is that much, at most, is bearable. The important thing is to document it properly. Makefile commands shell autocomplete helps a lot too.

Regarding the number of start-test that we have currently (19):

start-test-acceptance-frontend                        start-test-acceptance-frontend-seamless               start-test-acceptance-server-guillotina
start-test-acceptance-frontend-coresandbox            start-test-acceptance-frontend-seamless-multilingual  start-test-acceptance-server-multilingual
start-test-acceptance-frontend-coresandbox-dev        start-test-acceptance-frontend-workingcopy            start-test-acceptance-server-seamless-multilingual
start-test-acceptance-frontend-dev                    start-test-acceptance-server                          start-test-acceptance-server-workingcopy
start-test-acceptance-frontend-guillotina             start-test-acceptance-server-5                        start-test-acceptance-webserver-seamless
start-test-acceptance-frontend-multilingual           start-test-acceptance-server-coresandbox
start-test-acceptance-frontend-project                start-test-acceptance-server-detached

Number of acceptance- then tab (6):

acceptance-backend-start          acceptance-frontend-prod-start    acceptance-server-detached-stop
acceptance-frontend-dev-start     acceptance-server-detached-start  acceptance-test

In this regard, we improved, narrowing more the start of the command by using the "thing" at the beginning.

@tisto One last fact: this naming convention is in place in Cookieplone.

@stevepiercy
Copy link
Collaborator

@stevepiercy I find it a bit strange that you made changes to ensure that different words are split by - and yet in the table I see docs-linkcheckbroken copyreleasenotestodocs now they just seem out of place and it would be a shame not to have everything clean 99% of it is already :)

@ichim-david I'm glad you asked. And @tisto, here's the Volto (and Plone and cookiecutter) reality.

First a little background.

Volto has a total of 69 makefile commands with descriptions. 26 of them start a thing. There was no naming convention, and often had an inconsistent or vague description or none at all. Some had the action at the end, others at the beginning, and even a few in the middle. When coming up with a naming convention, we considered how to group them most effectively, in other words, using a taxonomy. There is solid basis for naming things taxonomically in the sciences, even in computer science. This guy does a much better job of describing the taxonomic hierarchical rationale than I ever could.

I would yearn to have only a few simple makefile commands—install|build|start|stop|restart—but with all the things Volto has and does, it's only possible for a few commands.

In documentation and many other projects, we don't use the docs- prefix. But in Volto, we needed some way to indicate their intended usage. Thus docs-* came to be. This was our first taxonomic group, where we put the name of the thing first and the action last.

Sometimes there can be exceptions. There are some non-compliant items.

When reviewing, sometimes we miss things. In my first PR review I missed runall, which should have been run-all. They may be more.

linkcheck is the program name of a Sphinx build. We ought to keep that name consistent with the binary because linkcheck is, after all, a literal thing. [docs-]linkcheckbroken could become [docs-]linkcheck-broken, though. It's one of those items I missed, as I am used to seeing it that way.

copyreleasenotestodocs was renamed to release-notes-copy-to-docs. If you see it somewhere in the old form, please point it out.

* main:
  Remove `api` folder. Add `backend` folder using latest backend best practices (#6110)
  Rename files with wrong extension `js->jsx` when they contain JSX. (#6114)
  Improve shadowing by including the support for `js->jsx` extensions in… (#6113)
  issue-5452 markdown shortcut (#5495)
@sneridagh
Copy link
Member Author

@tisto @ichim-david Could you weigh in and discuss on the latest comments? I'd love to have a final blessing in this one, as it is ready, and adjusted to the latests PRs. I'd like to have it merged asap.

@stevepiercy
Copy link
Collaborator

@sneridagh as a final step before merge, I want to check docs in both the Volto and Documentation repos for accuracy. We might need to do some versionchanged stanzas with the latest commands.

@ichim-david ichim-david self-requested a review June 23, 2024 07:48
@sneridagh
Copy link
Member Author

@stevepiercy what's left to be done? Can I help?

@stevepiercy
Copy link
Collaborator

@sneridagh we need to make sure that any instances of the old target names are renamed everywhere, including the docs. It's a tedious, yet necessary chore. I'll start on that now.

@stevepiercy
Copy link
Collaborator

@sneridagh done in 6b799d9

@sneridagh sneridagh merged commit 14c1ddb into main Jun 26, 2024
71 checks passed
@sneridagh sneridagh deleted the renamemakefiletargets branch June 26, 2024 06:37
sneridagh added a commit that referenced this pull request Jun 26, 2024
* main: (73 commits)
  Release 18.0.0-alpha.36
  Rename missing command
  Image widget PR as breaking (#6125)
  Release @plone/slate 18.0.0-alpha.14
  Release @plone/registry 1.7.0
  Rename Makefile targets (#6104)
  fix: nonContentRoutes diff path (#6102)
  Automatically set the label to `03 type: feature (plip)` for PLIPs (#6122)
  Add ImageWidget with upload/drop/external and inline/widget capabilities (#5607)
  Ensure that sidebar field will not steal focus when metadata is edited (#5983)
  Prevent duplicated UUUIDs in inner blocks when copying container blocks (#6112)
  feat: handle breakList in detached TextBlockEditor (#6106)
  Make dynamic/live teaser without additional requests (#6024)
  Fix issue of duplicated blocks upon pasting an image into the Slate E… (#5818)
  Remove `api` folder. Add `backend` folder using latest backend best practices (#6110)
  Rename files with wrong extension `js->jsx` when they contain JSX. (#6114)
  Improve shadowing by including the support for `js->jsx` extensions in… (#6113)
  issue-5452 markdown shortcut (#5495)
  Release 18.0.0-alpha.35
  Release @plone/types 1.0.0-alpha.16
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants