Skip to content

Conversation

@emgre
Copy link
Contributor

@emgre emgre commented Apr 15, 2020

Fix #55

I'm using this code in this GitHub Actions pipeline: https://github.com/dnp3/opendnp3/actions

I only tried it on Windows. Perhaps some integration tests could be added to verify that the proper version is fetched. I'm not sure to understand how your tests are run, so I didn't do it.

@emgre emgre force-pushed the bugfix/architecture branch from 6cd3630 to 8b062ac Compare April 15, 2020 15:33

let archExtension = '';
if (arch === 'x86') {
archExtension = 'i686';
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did this extension come from? Is it not x86?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i686 is the latest iteration of the IA-32 (commonly known as x86). From today's perspective, it's a synonym of x86 (although it is technically not). I didn't choose this extension, I just looked at https://cdn.azul.com/zulu/bin/ and saw that's what they use.

} else if (arch === 'x64') {
archExtension = 'x64';
} else {
throw new Error(`architecture "${arch}" is not int [x86 | x64]`);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should validate that the architecture input is valid when we retrieve the input in https://github.com/actions/setup-java/blob/master/src/setup-java.ts#L12

Then this whole block can just be:

const archExtension = arch === 'x86' ? 'i686' : 'x64';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. I'm not very familiar with the GitHub Actions core library (and JS in general) so bear with me. I'll make the change soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries! Let me know if you need any help

Choose a reason for hiding this comment

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

Should be "not in" instead of "not int".

@joshmgross joshmgross requested a review from konradpabjan April 29, 2020 15:46
@AustinShalit
Copy link
Contributor

Hi folks. Any progress updates on this PR?

@AustinShalit
Copy link
Contributor

@konradpabjan This PR can now be closed.

@joshmgross
Copy link
Contributor

Replaced with #95

@joshmgross joshmgross closed this Aug 24, 2020
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.

architecture parameter is ignored

4 participants