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

Wrong URL in the "recent commit" section #98

Open
QuentinLemCode opened this issue Sep 10, 2024 · 6 comments · May be fixed by #100
Open

Wrong URL in the "recent commit" section #98

QuentinLemCode opened this issue Sep 10, 2024 · 6 comments · May be fixed by #100

Comments

@QuentinLemCode
Copy link
Contributor

Hello

The URL in the "Recent commit" doesn't follow the permalink used in .cherry.js

It gave me an URL with the following format : https://github.com/${project_name}/commit/${sha1}

But my project name isn't the same as the github one.

Could we configure the repository name in the .cherry.js file ?

@fwuensche
Copy link
Contributor

Just setting a repository name wouldn't be enough as we still wouldn't know if it is on GitHub, GitLab, or something else. The ideal solution is the one we used for permalinks.

The problem, however, is that the link you mentioned is generated on the fly, in the back-end, where we don't have all the information coming from cherry config.

https://github.com/cherrypush/cherrypush.com/blob/6b98a62c0f785d48363d6d40f80b90eb33ea7c6d/app/helpers/application_helper.rb#L44-L45

I'm afraid that adding another configuration in the back-end will tend to overcomplicate things. So I'd tend towards a solution similar to permalink, where we add another option to the cherry config file, like below. Wdyt?

commitUrl: (commitSha) => `https://github.com/upflow/upflow/commit/${commitSha}`

@QuentinLemCode
Copy link
Contributor Author

That's fine for me :)

@fwuensche
Copy link
Contributor

Actually, the above solution won't be enough. I'll reach out in private to discuss other ideas.

@fwuensche
Copy link
Contributor

fwuensche commented Sep 18, 2024

Okay, I'm considering the following changes and would love your feedback.
I'll outline them here and let it sit for a few days before implementing.

I believe a basic config file should contain only the essential elements—metrics and plugins:

{
  plugins: { ... }, 
  metrics: [{ ... }]
  // ^ it makes me think plugins should be an array too, but let's keep it for another thread
}

In other words, you don’t even need to set a project name. Cherry will automatically infer it from your Git remote origin.

Behind the scenes, Cherry will gather repository information like this:

{
  repository: {
    host: 'github', // inferred from git remote origin
    owner: 'cherrypush', // inferred from git remote origin
    name: 'cherrypush.com', // inferred from git remote origin
    subdir: 'frontend', // calculated by comparing the config file path to `git rev-parse --show-toplevel`
  },
  plugins: { ... },
  metrics: [{ ... }]
}

While this data won’t be required in the config, you’ll still have the option to manually set it if needed.

The main benefits I see are:

  • (1) Retro-compatible: Existing projects will work without any required changes.
  • (2) Out-of-the-box: New users won’t need to configure project names or other details.
  • (3) Flexible: You can still customize all of these parameters if desired, but it’s optional

Let me know if you spot any potential issues or think some parameter names should be changed.
I’ll likely start working on it next weekend. 🏋️

@QuentinLemCode
Copy link
Contributor Author

QuentinLemCode commented Sep 19, 2024

It seems great, it would give everything you need in order to generate URLs.

How will you make the difference between Gitlab and Github (or other) ? By using the "host" value ?

@fwuensche
Copy link
Contributor

Yes, based on the host, we can probably compose the all the URLs following a specific pattern. And there are not so many hosts, so it's ok. I'll open a PR so we can review this implementation.

@fwuensche fwuensche linked a pull request Sep 19, 2024 that will close this issue
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 a pull request may close this issue.

2 participants