Add a cat command and note in the build output when a config file is properly used.

[ericholscher]

This is important for our config file migration,
so users are given feedback when they have properly configured the file.

This does two things:

• Adds a cat <config_file> to the build output, so users know a config is used, and which one
• Adds a small message above the build to share that the config file was used successfully. I will plan to remove this once our config file migration is over.

[humitos]

If we want to merge this message, I'd propose to just tell people their project is configured correctly and that we found a configuration file and remove the mention of the path in particular.

Otherwise, I would say we should output the cat YAML file as an extra command in the list of commands ran.

[humitos]

Be careful with this 😄 . Note that it could have been readthedocs.yaml or readthedocs.yml or .readthedocs.yml. We don't know at this point 🙃

Maybe we should not mention this here and just output the cat <path-to-yaml> as you mentioned in Slack for the case where the config file was found.

[agjohnson]

Yeah, I'd opt for putting this in the build output as well. I've been trying to get us away from making one-off UI blocks like this as they tend to overload the page design. I think putting it in the build output as cat .readthedocs.yaml makes sense and gives us a command to point the user to.

It also serves as a way to show the user what configuration was detected. The beta dashboard shows the rendered version in addition, but I commonly want to see both.

[agjohnson]

Also, I'll echo feedback I had on this in another issue: I'd like to user fewer patterns to give the user feedback and ideally stick to a single pattern. For now, this pattern has been integrating the debug output in the command listing.

We did talk about how to redo our notification system, which is work I keep raising as it's a pretty base part of our user interaction with our UI. I'll continue this discussion in a new issue and hopefully we can put some priority on it.

[benjaoming]

Noting that the upcoming configuration file how-to also relies on some feedback in the UI. It's pretty hard to instruct people about what the successful outcome looks like if they can't see that a configuration file is used in their builds.

[benjaoming]

Adding another idea from the how-to work: It would be great if we could use a small screenshot from the build page so people know exactly what the successful outcome looks like. And the build page for that matter! Someone reading this how-to might not have even used the build page yet!

[ericholscher]

So it sounds like folks are 👍 on adding a cat {{ build.readthedocs_yaml_path }} to the build process as a "debug" build? If folks are 👍 , I can get that ready this week.

[humitos]

I'm fine with the "cat" command, yeah

[benjaoming]

@ericholscher yes that's perfect, this way we can both solve the issue of knowing that the config file is in use and knowing the content of the config file without extra clicks to the user's repo (which might be also private).

Small note: You don't even have to worry if the file path is correct, that's already handled inside the builder with a nice error message. Everything else can go in a follow-up issue.

Nice with that build.readthedocs_yaml_path 😇

[ericholscher]

@benjaoming Yea, but that is only set when there's a custom path right? So I still need to get the data from the config file parsing around the exact path of the file, I believe.

[benjaoming]

@ericholscher

Yes, I think it would be a timely intervention to just start storing the config file used by a build no matter what.

This:

readthedocs.org/readthedocs/doc_builder/director.py

Line 241 in 8012b88

self.data.build["readthedocs_yaml_path"] = custom_config_file

Should be something like:

self.data.build["readthedocs_yaml_path"] = self.data.config.source_file

Edit: Oh no, we are setting source_file to something that isn't a source file here 😢

readthedocs.org/readthedocs/doc_builder/config.py

Lines 65 to 73 in 8012b88

	except DefaultConfigFileNotFound:
	# Default to use v1 with some defaults from the web interface
	# if we don't find a configuration file.
	config = BuildConfigV1(
	env_config=env_config,
	raw_config={},
	source_file=checkout_path,
	)
	config.validate()

[ericholscher]

@stsewd do you have thoughts on the best way to pass the actual config file parsed back up to a level where we can output a build command? Should we just trust the source_file on the config object, and perhaps change the fallback that Benjamin noted above to be more obvious that it's injected?

[stsewd]

@stsewd do you have thoughts on the best way to pass the actual config file parsed back up to a level where we can output a build command? Should we just trust the source_file on the config object, and perhaps change the fallback that Benjamin noted above to be more obvious that it's injected?

Passing a directory as source file is a way to pass the base path together with the config file, I think we can just pass that separately, have a base_path and a source_file. In case of using the web interface, the source file can be None, but the base path will be the checkout path.

[ericholscher]

This PR is ready for another review. I did a quick version of the base_path update, but didn't require it since I assume that would have broken a bunch of tests 🙃

I'm also fine removing the messaging on the build completely, but I think having a very visible ✅ there is useful for the migration, and not just "look for cat <yaml> in your build output" as the way to confirm config file usage.

[agjohnson]

Do you have a screenshot of the warning block on community and commercial?

It's going to feel a bit redundant showing it on every build, especially where we have a warning if the project isn't using a configuration file. Referencing the warning seems reasonable too

[ericholscher]

It's going to feel a bit redundant showing it on every build, especially where we have a warning if the project isn't using a configuration file. Referencing the warning seems reasonable too

That makes sense, "ensure you have a cat <config> command, and no big warning. Simple enough, and probably fine if we're worried about the styling of it.

.org

[humitos]

Just a small comment about the path shown to the user.

[humitos]

Looks good to me. I just left some more comments to keep consistency with other parts of the code.

Also, I don't understand why tests are passing if they were not updated. I would have expected one of the tests that run the "full build" to fail saying there is a new command in the list of commands.

[humitos]

I don't think we need this replacement here. This should is done automatically at the API when retrieving the commands. We are storing the "full command" including the long path in the database.

Suggested change

	self.data.config.source_file.replace(
	checkout_path + "/", ""
	),
	self.data.config.source_file,

• API:

  readthedocs.org/readthedocs/api/v2/serializers.py

  Lines 177 to 180 in 07ef84f

  	def get_command(self, obj):
  	return normalize_build_command(
  	obj.command, obj.build.project.slug, obj.build.version.slug
  	)

• Replacement function:

  readthedocs.org/readthedocs/api/v2/utils.py

  Lines 265 to 292 in 07ef84f

  	def normalize_build_command(command, project_slug, version_slug):
  	"""
  	Sanitize the build command to be shown to users.
  	It removes internal variables and long paths to make them nicer.
  	"""
  	docroot = settings.DOCROOT.rstrip("/") # remove trailing '/'
  	# Remove Docker hash from DOCROOT when running it locally
  	# DOCROOT contains the Docker container hash (e.g. b7703d1b5854).
  	# We have to remove it from the DOCROOT it self since it changes each time
  	# we spin up a new Docker instance locally.
  	container_hash = "/"
  	if settings.RTD_DOCKER_COMPOSE:
  	docroot = re.sub("/[0-9a-z]+/?$", "", settings.DOCROOT, count=1)
  	container_hash = "/([0-9a-z]+/)?"
  	regex = f"{docroot}{container_hash}{project_slug}/envs/{version_slug}(/bin/)?"
  	command = re.sub(regex, "", command, count=1)
  	# Remove explicit variable names we use to run commands,
  	# since users don't care about these.
  	regex = r"^\$READTHEDOCS_VIRTUALENV_PATH/bin/"
  	command = re.sub(regex, "", command, count=1)
  	regex = r"^\$CONDA_ENVS_PATH/\$CONDA_DEFAULT_ENV/bin/"
  	command = re.sub(regex, "", command, count=1)
  	return command

[ericholscher]

@humitos when I tested it locally it didn't replace the path. I think this is because it isn't including the bin or envs path because it's not an executable? We could expand this logic to remove that path from anywhere in the string, but I didn't want to touch too much code.

[humitos]

That's fine for now. Can you add a TODO comment in the code where you are replacing this so we don't forget about this and come back?