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

fixes #477 (Build should not rely on git) #481

Merged
merged 3 commits into from Dec 17, 2023

Conversation

satti-hari-krishna-reddy
Copy link
Contributor

@satti-hari-krishna-reddy satti-hari-krishna-reddy commented Dec 8, 2023

fixes #477

Description :

This pull request addresses the issue where the build process was relying on Git for version information. The problem arose when the build process did not have Git available.

Changes Made :

-In package.json, updated the version field to the latest released GitHub version

-In build.ts, modified the code to use the version specified in package.json.

@satti-hari-krishna-reddy
Copy link
Contributor Author

@sushain97 could you please review my changes? Your feedback would be highly appreciated.

Copy link
Member

@sushain97 sushain97 left a comment

Choose a reason for hiding this comment

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

Seems reasonable - some questions below.

@@ -4,7 +4,7 @@
"author": "Sushain Cherivirala <sushain@skc.name>",
"license": "GPL-3.0-or-later",
Copy link
Member

Choose a reason for hiding this comment

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

Can you update the version field above to actually match the latest released GH version if we're going to use it as the canonical source?

package.json Outdated
@@ -4,7 +4,7 @@
"author": "Sushain Cherivirala <sushain@skc.name>",
"license": "GPL-3.0-or-later",
"scripts": {
"build": "ts-node build.ts",
Copy link
Member

Choose a reason for hiding this comment

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

Why do environment variables need to be manually passed through? As far as I'm aware, yarn doesn't modify them from the caller.

@coveralls
Copy link

coveralls commented Dec 11, 2023

Pull Request Test Coverage Report for Build 7208461184

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 97.64%

Totals Coverage Status
Change from base Build 7013115272: 0.0%
Covered Lines: 1380
Relevant Lines: 1395

💛 - Coveralls

@satti-hari-krishna-reddy
Copy link
Contributor Author

satti-hari-krishna-reddy commented Dec 12, 2023

@sushain97 sorry for late reply college work has been busy, Sure, I'll update the version in package.json field to actually match the latest released git hub version but i got a doubt , Since we're moving away from direct reliance on Git ,will manual updates to package.json be required for every release?

@TinoDidriksen
Copy link
Member

...will manual updates to package.json be required for every release?

Yes. That is the point. It's about what the source of truth is, and I hold that it should be the code - GitHub is merely storage, and must not be an integral part of the project.

@sushain97
Copy link
Member

Since we're moving away from direct reliance on Git ,will manual updates to package.json be required for every release?

Yes, this is fine. Ideally, we set up a GH workflow to create a tag and it'll fail when the tag already exists and succeed when a commit is pushed to master with a new tag.

@sushain97
Copy link
Member

Yes. That is the point. It's about what the source of truth is, and I hold that it should be the code - GitHub is merely storage, and must not be an integral part of the project.

I don't disagree with this but I'd argue GitHub isn't relevant here. The tags are a Git construct which is arguably part of the source of truth. The point that a build shouldn't require .git is one I agree with though so this is fine in general.

@satti-hari-krishna-reddy
Copy link
Contributor Author

Hey @sushain97 , @TinoDidriksen , I've made some changes to the code. Can you take a look and let me know if everything's good now? Thanks!

sushain97
sushain97 previously approved these changes Dec 14, 2023
@sushain97
Copy link
Member

@satti-hari-krishna-reddy CI is failing.

@satti-hari-krishna-reddy
Copy link
Contributor Author

@sushain97 , I've got the Prettier error fixed, so the CI should be good to go now!

@sushain97 sushain97 merged commit c8c7b20 into apertium:master Dec 17, 2023
4 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.

Build should not rely on git
4 participants